Bug 64250 - Global strict mode function leaking global object as "this".
: Global strict mode function leaking global object as "this".
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 64677 64678 64679
  Show dependency treegraph
 
Reported: 2011-07-10 22:11 PST by
Modified: 2011-12-25 15:49 PST (History)


Attachments
Updated test case (1.18 KB, text/html)
2011-07-13 14:12 PST, Gavin Barraclough
no flags Details
Correction to test case (1.18 KB, text/html)
2011-07-13 14:19 PST, Gavin Barraclough
no flags Details
The patch (51.94 KB, patch)
2011-07-14 12:45 PST, Gavin Barraclough
oliver: review-
Review Patch | Details | Formatted Diff | Diff
Fix for global object getter/setter properties, added test case & ASSERT in JSC::call to guard against global object being passed. (55.65 KB, patch)
2011-07-14 15:30 PST, Gavin Barraclough
oliver: review-
Review Patch | Details | Formatted Diff | Diff
Added asserts, changed calls in WebCore/WebKit/WebKit2 to pass correct global object to call. (66.98 KB, patch)
2011-07-15 12:00 PST, Gavin Barraclough
oliver: 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 2011-07-10 22:11:28 PST
> function foo(){"use strict"; return this;}
    undefined

    > foo()
    DOMWindow

The call to foo() should have returned undefined.
------- Comment #1 From 2011-07-12 22:44:26 PST -------
Trivially, the bug here is that JSGlobalObject doesn't set NeedsThisConversion.  But just setting that may be a performance regression.  (Will make us take unnecessary calls out in non-strict code).

An alternative would be a NeedsStrictThisConversion flag, but it would probably be better to fix jsc so we no longer rely on this conversion to fix up scope objects once we're inside the call, and instead to pass the correct this object into functions in the first place.
------- Comment #2 From 2011-07-13 12:23:52 PST -------
http://code.google.com/p/v8/issues/detail?id=1547 has a testcase in comment 6, correct ES5 output is "oooouuuu", Firefox does "ioiouuuu" for backward compat (but we would like to break that in a coordinated move by browser vendors, with site evangelism as needed).

/be
------- Comment #3 From 2011-07-13 14:12:11 PST -------
Created an attachment (id=100706) [details]
Updated test case
------- Comment #4 From 2011-07-13 14:19:21 PST -------
Created an attachment (id=100709) [details]
Correction to test case
------- Comment #5 From 2011-07-13 14:37:09 PST -------
(In reply to comment #2)
> http://code.google.com/p/v8/issues/detail?id=1547 has a testcase in comment 6, correct ES5 output is "oooouuuu", Firefox does "ioiouuuu" for backward compat (but we would like to break that in a coordinated move by browser vendors, with site evangelism as needed).
> 
> /be

Hey Brendan,

Thanks for the test case.  Just to confirm, the text in the test case seems to be incorrect in stating the expected result to be "iooouuuu", whereas my understanding agrees with your comment that the correct result should be "oooouuuu".

I've expanded the test to cover 'with' behaviour.  I'd expected that the correct this value to be passed in all cases of with statements should be the base object that the property is being accessed off, but looking at the spec (and Safari/FireFox behaviour!) this is incorrect, since the definition for the comma operator causes GetValue to be called on the reference.  It would be great to get confirmation that my understanding here is correct!

WRT to coordinating releases, that would be great.  I'll probably fix this in ToT soon so we can start the bug finding /evangelism process.  I think you guys have plans on a faster release cycle than we do right now - any idea when you'd expect to see this change in a release Firefox?

many thanks,
G.
------- Comment #6 From 2011-07-13 16:24:26 PST -------
(In reply to comment #5)
> (In reply to comment #2)
> > http://code.google.com/p/v8/issues/detail?id=1547 has a testcase in comment 6, correct ES5 output is "oooouuuu", Firefox does "ioiouuuu" for backward compat (but we would like to break that in a coordinated move by browser vendors, with site evangelism as needed).
> > 
> > /be
> 
> Hey Brendan,
> 
> Thanks for the test case.  Just to confirm, the text in the test case seems to be incorrect in stating the expected result to be "iooouuuu", whereas my understanding agrees with your comment that the correct result should be "oooouuuu".

ES5 wants oooouuuu.

Mark, upon hearing the full horror of the web compatibility constraint, wants iooouuuu (he wrote the test, I extended it to test the strict callee g). This is something Mark wants to allow strict callers to protect themselves from leaking the capability to their global object to non-strict callees.

Firefox and IE do ioio.... (IE, even IE10pp, still does not handle strict mode correctly, so gives ioio for .... -- Firefox gives uuuu).

For SpiderMonkey in Firefox, we avoided an extra test for strict caller and wound up with ioiouuuu. We could add the test to avoid the capability leak, but I just today expressed to mark my hope that we can all switch to oooouuuu without breaking the web (maybe with a bit of site evangelism).

> I've expanded the test to cover 'with' behaviour.  I'd expected that the correct this value to be passed in all cases of with statements should be the base object that the property is being accessed off, but looking at the spec (and Safari/FireFox behaviour!) this is incorrect, since the definition for the comma operator causes GetValue to be called on the reference.  It would be great to get confirmation that my understanding here is correct!

You are correct.

> WRT to coordinating releases, that would be great.  I'll probably fix this in ToT soon so we can start the bug finding /evangelism process.  I think you guys have plans on a faster release cycle than we do right now - any idea when you'd expect to see this change in a release Firefox?


Firefox 8, so since we shipped 5 and we ship every quarter, < 3 quarters from now -- early next year.

Question is how many sites will test with our nightly, aurora (think a for alpha) and beta channels, which pipeline-stage every six weeks. But if we get you and V8 to do oooouuuu as well, then *maybe*. The IE share is the worry.

/be
------- Comment #7 From 2011-07-14 12:45:41 PST -------
Created an attachment (id=100846) [details]
The patch

Perf tested okay, tested 32bit, 64bit, DFG & interpreter builds.
------- Comment #8 From 2011-07-14 12:55:34 PST -------
(From update of attachment 100846 [details])
Check behaviour of getters and setters on the global object, including the cached scenarios.
------- Comment #9 From 2011-07-14 15:30:57 PST -------
Created an attachment (id=100875) [details]
Fix for global object getter/setter properties, added test case & ASSERT in JSC::call to guard against global object being passed.
------- Comment #10 From 2011-07-14 15:39:55 PST -------
(From update of attachment 100875 [details])
r=me
------- Comment #11 From 2011-07-14 15:47:40 PST -------
(From update of attachment 100875 [details])
I think badness can happen in ::execute(Program... ::execute(EvalNode
------- Comment #12 From 2011-07-15 12:00:06 PST -------
Created an attachment (id=101020) [details]
Added asserts, changed calls in WebCore/WebKit/WebKit2 to pass correct global object to call.
------- Comment #13 From 2011-07-15 12:11:21 PST -------
Attachment 101020 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/interpreter/Interpreter.h:100:  The parameter name "scopeChain" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #14 From 2011-07-15 12:12:02 PST -------
(From update of attachment 101020 [details])
r=me
------- Comment #15 From 2011-07-15 12:53:32 PST -------
I've landed these changes in r91095, Now we just need be vigilant for compatibility issues.

G.
------- Comment #16 From 2011-07-16 20:33:55 PST -------
This caused a bunch of new assertions on the bots:
http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r91161%20(32455)/http/tests/inspector/network/network-embed-crash-log.txt
------- Comment #17 From 2011-07-17 14:12:53 PST -------
(In reply to comment #16)
> This caused a bunch of new assertions on the bots:
> http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r91161%20(32455)/http/tests/inspector/network/network-embed-crash-log.txt

Cheers Simon, missed those, will investigate.
------- Comment #18 From 2011-07-20 19:18:52 PST -------
*** Bug 51097 has been marked as a duplicate of this bug. ***
------- Comment #19 From 2011-07-20 19:19:01 PST -------
*** Bug 58338 has been marked as a duplicate of this bug. ***
------- Comment #20 From 2011-08-11 17:09:12 PST -------
The isValidThisObject assert added to Source/JavaScriptCore/runtime/CallData.cpp breaks the Qt JS bridge signal connection functionality in debug builds.
http://doc.qt.nokia.com/latest/qtwebkit-bridge.html#signal-to-function-connections

See bug 66097.
------- Comment #21 From 2011-12-25 15:49:26 PST -------
*** Bug 11884 has been marked as a duplicate of this bug. ***