code review requested - IE7 patch

[John Tamplin]

This patch is a gross hack to work around a problem with IE7 hosted mode.  When a JavaScript exception is caught and returned to Java, some JS wrapper code catches the exception and extracts the number, name, and message fields.  In IE7, the name field results in a Variant with a type of 130, which I can't find documented anywhere, and the hosted mode code refuses to accept this as a string.  Attempts to convert it to a string in JavaScript (using String( e.name), (e.name + "") and e.name.toString()) failed to produce a string, so for the short term I propose this patch as a bandaid to the problem.  This patch will prevent IE7 from causing failing tests and will only affect the name field of the resulting exception, which is likely to only be used for human consumption anyway.  So, I think this is reasonable for now while we work on the real solution.

--
John A. Tamplin
Software Engineer, Google

[Bruce Johnson]

I'm torn. I think patching it to stop unit tests failing is a pretty risky practice since the failure is telling us something important. In particular, since we don't understand exactly what's happening, we can't say for sure that it won't happen elsewhere.

I would rather spend another day aggressively trying to figure this out than to hack it. Did you try getting the latest Microsoft Platform SDK? Surely it's in the headers somewhere. Maybe online MSDN is just old and doesn't explain the VARENUM. Maybe the source itself would explain.

Index: dev/windows/src/com/google/gwt/dev/shell/ie/JsValueIE6.java
===================================================================
--- dev/windows/src/com/google/gwt/dev/shell/ie/JsValueIE6.java (revision 679)
+++ dev/windows/src/com/google/gwt/dev/shell/ie/JsValueIE6.java (working copy)
@@ -117,7 +117,17 @@
   * @see com.google.gwt.dev.shell.JsValue#getString()
   */
  public String getString() {
-    return variant.getString();
+    // TODO(jat): gross hack for IE7 support:  IE7 appears to return an
+    // object of type 130, which is not documented anywhere, from the name
+    // field of a JavaScript exception.  We special-case this type to allow
+    // return a marker string instead of failing.  We need to figure out the
+    // proper way to get the name for IE7, but for now this hack keeps tests
+    // from failing.
+    if (variant.getType() == 130) {
+      return "IE7 Exception Name";
+    } else {
+      return variant.getString();
+    }
  }

  /*
@@ -260,8 +270,14 @@
    if (variant.getType() == COM.VT_BSTR) {
      return true;
    }
-    if (variant.getType() != COM.VT_DISPATCH) {
-      return false;
+    // TODO(jat): gross hack for IE7 support:  IE7 appears to return an
+    // object of type 130, which is not documented anywhere, from the name
+    // field of a JavaScript exception.  We special-case this type to allow
+    // return a marker string instead of failing.  We need to figure out the
+    // proper way to get the name for IE7, but for now this hack keeps tests
+    // from failing.
+    if (variant.getType() == 130) {
+      return true;
    }

    // see if it's a String wrapper object

[John Tamplin]

On 3/23/07, Bruce Johnson <br...@google.com> wrote:

I'm torn. I think patching it to stop unit tests failing is a pretty risky practice since the failure is telling us something important. In particular, since we don't understand exactly what's happening, we can't say for sure that it won't happen elsewhere.

It isn't just to make tests stop failing, but also to make IE7 usable for hosted mode.  Right now, you can't even usefully debug your code on Windows if you have IE7 installed and JavaScript throws an exception.  I think some fix to enable debugging is necessary by the point we make any sort of distribution from the current trunk.  It's not like I was saying this shouldn't be fixed for 1.4, but rather getting something that solves the immediate problem so we know we don't have other problems and then during the bugfix days find the real solution.

I agree that IE7 (or a future patch) could return this weird object for something else, but presumably our tests would catch getting a bogus value back for some operation, like they did in this case.

I would rather spend another day aggressively trying to figure this out than to hack it. Did you try getting the latest Microsoft Platform SDK? Surely it's in the headers somewhere. Maybe online MSDN is just old and doesn't explain the VARENUM. Maybe the source itself would explain.

Toby also looked, but I have no idea what version he was looking at.  He did ask if you had the latest version, so I presume he did have it.

I would be happy to work with someone else who is more familiar with Microsoft internals to find a real solution, but I feel by myself I would just be wading around without any real direction.

[Scott Blum]

Bruce's comments notwithstanding:

What happens when you try to use the Variant class to coerce type 130
into a String? Is there no way to get the actual name out? I thought
you said the correct type name (e.g. "Type Error") appeared in the
debugger.

What happened to these lines?
if (variant.getType() != COM.VT_DISPATCH) {
return false;
}
Shouldn't they still be in there after the 130 hack?

Scott

On 3/22/07, John Tamplin <j...@google.com> wrote:

[John Tamplin]

On 3/23/07, Scott Blum <sco...@google.com> wrote:

What happens when you try to use the Variant class to coerce type 130
into a String?  Is there no way to get the actual name out?  I thought
you said the correct type name (e.g. "Type Error") appeared in the
debugger.

You mean in Java variant.toString()?  I can try that.  I tried everything I could think of in JS to no effect - it was still getting a type 130 variant passed into the Java code.  Even if that works, I would still have to special-case type 130 in isString, unless you want to go back to allowing any variant that can be converted to a string to substitute transparently for one.

I saw briefly Miguel had MSVSP debugging that code and for the name parameter in JS it had TypeError.  He had to go do something else so we didn't follow up on that further.  I had not reinstalled MSVSP in my VMware instance since things were rearranged, but I installed it yesterday so I can look at that myself today.

What happened to these lines?
  if (variant.getType() != COM.VT_DISPATCH) {
    return false;
  }
Shouldn't they still be in there after the 130 hack?

I will check when I get in (I can't get into my Windows VMware instance from home), but yes the handling of String should not have been changed.  It was probably removed by accident hand-editing the diff file to remove the still-pending change to factor out the code for handling String objects since I didn't want to mix that change in with this one for review.

[Scott Blum]

On 3/23/07, John Tamplin <j...@google.com> wrote:
> On 3/23/07, Scott Blum <sco...@google.com> wrote:
> > What happens when you try to use the Variant class to coerce type 130
> > into a String? Is there no way to get the actual name out? I thought
> > you said the correct type name (e.g. "Type Error") appeared in the
> > debugger.
>
> You mean in Java variant.toString()? I can try that. I tried everything I
> could think of in JS to no effect - it was still getting a type 130 variant
> passed into the Java code. Even if that works, I would still have to
> special-case type 130 in isString, unless you want to go back to allowing
> any variant that can be converted to a string to substitute transparently
> for one.

I'm just saying, returning "IE7 Exception Name" is about the least
useful thing we can do, I'm sure you can find a good solution to doing
something better. :)

> > What happened to these lines?
> > if (variant.getType() != COM.VT_DISPATCH) {
> > return false;
> > }
> > Shouldn't they still be in there after the 130 hack?
> >
>
> I will check when I get in (I can't get into my Windows VMware instance from
> home), but yes the handling of String should not have been changed. It was
> probably removed by accident hand-editing the diff file to remove the
> still-pending change to factor out the code for handling String objects
> since I didn't want to mix that change in with this one for review.

That's bitten you before, I think. FYI, here's my workflow, when I
have multiple changes that may conflict:

I'll make a patch file for my first change set (in the root of the
trunk). Then I'll revert those changed files, get my working copy
back into clean state, and begin working on the other change set.
That will become its own patch file. So generally I won't keep two
change sets active in the same client unless they clearly don't
conflict.

Of course, TortoiseSVN makes all this much easier, because it's got
UI-driven patch/apply patch that makes it easy to select a set of
files.

Scott

[John Tamplin]

On 3/23/07, Scott Blum <sco...@google.com> wrote:

I'm just saying, returning "IE7 Exception Name" is about the least
useful thing we can do, I'm sure you can find a good solution to doing
something better. :)

I'll see what I can do.

Of course, TortoiseSVN makes all this much easier, because it's got
UI-driven patch/apply patch that makes it easy to select a set of
files.

It is complicated by the fact that I want to test them together since my intent is to get them all into the trunk.

[Scott Blum]

On 3/23/07, John Tamplin <j...@google.com> wrote:

> > Of course, TortoiseSVN makes all this much easier, because it's got
> > UI-driven patch/apply patch that makes it easy to select a set of
> > files.
> >
> It is complicated by the fact that I want to test them together since my
> intent is to get them all into the trunk.

Sure, but this is where the law of useful partials comes into play.
Each change should be independently useful.. if they aren't, then it's
really just one change, right?

Scott

[John Tamplin]

On 3/23/07, Bruce Johnson <br...@google.com> wrote:

I'm torn. I think patching it to stop unit tests failing is a pretty risky practice since the failure is telling us something important. In particular, since we don't understand exactly what's happening, we can't say for sure that it won't happen elsewhere.

I would rather spend another day aggressively trying to figure this out than to hack it. Did you try getting the latest Microsoft Platform SDK? Surely it's in the headers somewhere. Maybe online MSDN is just old and doesn't explain the VARENUM. Maybe the source itself would explain.

Ok, here is another patch that I think is the best we can do without a ton of work.  At Scott's suggestion, I tried poking around in the 16 bytes of the underlying variant object and found a pointer to a string that corresponds to the string shown by MSVSP.  It is a string constant, so it isn't as useful as the name field in other browsers, but if that is what MS's own debugger shows then I don't know that we can do any better.  It still feels brittle as we are using an undocument constant and an undocumented field in the variant type, but I'm not sure how to handle it otherwise.

Attached is the patch, which also includes refactoring the class name lookup out of isString that has been floating around for a while.  I left the formatting alone in Variant.java even though it doesn't conform to our style guide since that is what was there before.

[Scott Blum]

In JsValueIE6:
It seems weird to return VT_UNKNOWN from getDispatchValueOf()... it
seems cleaner to assert (variant.getType() == VT_DISPATCH) inside of
the method, and guard outside of the method.

In Variant:
- You should use tabs instead of spaces in this file. Yes, this is
annoying, but it's how the rest of the file is formatted.
- You should mark the file with a "// Modfied by Google" comment in
the header, and mark all our mods with "GOOGLE:" comments. See
OleAutomation or Library for examples.
- You might consider refactoring the 130 out into a constant, which
you can give a descriptive name and put the main commenting on.
- You should also send a note to Steve Northover
<Steve_N...@ca.ibm.com> letting him know what we encountered and
how we're working around it. Please cc me on the thread.

Scott

On 3/23/07, John Tamplin <j...@google.com> wrote:

[John Tamplin]

On 3/23/07, Scott Blum <sco...@google.com> wrote:

In JsValueIE6:
It seems weird to return VT_UNKNOWN from getDispatchValueOf()... it
seems cleaner to assert (variant.getType() == VT_DISPATCH) inside of

the method, and guard outside of the method.

As discussed face-to-face, I am no longer factoring out this code and instead just added more comments to clarify it.

In Variant:
- You should use tabs instead of spaces in this file.  Yes, this is
annoying, but it's how the rest of the file is formatted.

- You should mark the file with a "// Modfied by Google" comment in
the header, and mark all our mods with "GOOGLE:" comments.  See
OleAutomation or Library for examples.
- You might consider refactoring the 130 out into a constant, which
you can give a descriptive name and put the main commenting on.

Done.  Also, as discussed FTF, I changed it so Variant.getData() changes the type to BSTR to minimize changes elsewhere.

- You should also send a note to Steve Northover
< Steve_N...@ca.ibm.com> letting him know what we encountered and
how we're working around it.  Please cc me on the thread.

I will send that on Monday.

Committed as r685.