Bug 33057 - REGRESSION(r49365): typeof(xhr.responseText) != "string" in Windows
: REGRESSION(r49365): typeof(xhr.responseText) != "string" in Windows
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: PC Windows 7
: P1 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-12-30 09:51 PST by
Modified: 2010-01-07 16:17 PST (History)


Attachments
proposed fix (24.71 KB, patch)
2010-01-07 15:42 PST, Alexey Proskuryakov
ggaren: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-12-30 09:51:18 PST
Hey guys.. this is a really odd one we noticed in the latest builds of the Win32/Cairo port, Brent Fulgham also verified this is happening in the official Windows port.

Basically in an XHR response, the responseText is correctly filled with data, but the "typeof" the responseText won't correctly compare against the string "string", unless the typeof or the responseText itself is forced into a String context. To clarify, here are a few examples:

"type="+typeof(xhr.responseText) // -> type=string

typeof(xhr.responseText) == "string" // false, should be true

String(typeof(xhr.responseText)) == "string" // true

typeof(String(xhr.responseText)) == "string" // true

var t = typeof(xhr.responseText);
t == "string" // true .. weird right?

tyepof(xhr.responseText).substr(0,5) == "string" // true

It should be noted that performing any standard String operations on the responseText or the typeof seem to work fine.

If you load this test case in WinLauncher, you should see all red text with typeof(responseText) == "string" ? false :
http://arcaner.com/test/test.html
------- Comment #1 From 2009-12-30 17:58:34 PST -------
Just to clarify, did this work before? Can you check when exactly this broke (which is the latest nightly build where this works)?

Sounds like this could be related to recent JavaScript string refactoring work. I can't reproduce this with nightly r52591 on Mac OS X, and didn't try on Windows though.
------- Comment #2 From 2009-12-31 19:07:22 PST -------
A little more information: we are only seeing this on the Windows builds. Neither the Mac build or the Linux GTK+ port show this behavior. I'm not sure how long this bug has been around, but I remember seeing it for at least a month. We can try to narrow it down a little.
------- Comment #3 From 2010-01-01 19:31:40 PST -------
I cannot reproduce this on Windows with nightly r52686. Please attach a complete reduced test case.
------- Comment #4 From 2010-01-05 10:48:59 PST -------
It could be that this is confined to the Win32 Cairo port. I'll double-check with the original reporter and update this ticket. Sorry for the confusion.
------- Comment #5 From 2010-01-06 16:07:40 PST -------
I can reproduce this now. I don't know what went wrong with my testing before. The only difference I'm aware of is that I was testing with XP, and am testing with Vista now - but that shouldn't affect behavior.

I'm working on a fix.
------- Comment #6 From 2010-01-06 19:48:38 PST -------
The problem is due to the fact that jsString() and related functions are now inline, so JSString objects constructed from WebCore get a different virtual function table pointer. This breaks isJSString() function. I have added appropriate assertions to constructors, and saw them fire:

ASSERT(vptr() == globalData->jsStringVPtr);

The common way of telling the compiler to use the same vtable in DLL and EXE is to use __declspec(dllimport/dllexport). And this fixes the assertion indeed (that's equivalent to what we have on Mac OS X). But it doesn't fix the actual bug!

Turns out that MSVC changes the vptr AFTER constructor exits, trying to fix bugs in certain kinds of code for programmers: <http://groups.google.com/group/microsoft.public.vc.language/msg/55cdcefeaf770212>. I couldn't find a way to disable this behavior. Some possible ways to fix this are:
- don't construct inline (or only do that when building JSC);
- fix vptr in jsString() after constructing the object;
- check for type in some other way (maybe typeid().name, like dynamic_cast does? I'm not sure if that works without RTTI though, or if MSVC breaks typename identity across DLL boundaries, too).

Suggestions are welcome.
------- Comment #7 From 2010-01-07 15:42:47 PST -------
Created an attachment (id=46093) [details]
proposed fix
------- Comment #8 From 2010-01-07 15:47:40 PST -------
Attachment 46093 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/cf/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/runtime/JSString.h:359:  More than one command on the same line  [whitespace/newline] [4]
JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 9
------- Comment #9 From 2010-01-07 15:58:20 PST -------
(From update of attachment 46093 [details])
r=me
------- Comment #10 From 2010-01-07 16:17:32 PST -------
Committed <http://trac.webkit.org/changeset/52956>.

This bug also affected PaceKeeper benchmark for Windows Safari, which now works for me.

For the record, the issue I had with my initial testing was that I was using Safari 4.0.3 - after updating to 4.0.4 on that machine, I could reproduce.