Bug 87158 – REGRESSION (r106478): None of the Paper.js JavaScript examples work

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Bastien Dejean]

None of the JavaScript examples from Paper.js (see URL) work.
The error message doesn't make sense: "ReferenceError: Can't find variable: ...".

Arch Linux, libwebkit 1.8.1.

[Alexey Proskuryakov]

Confirmed on Mac, regressed in <http://trac.webkit.org/changeset/106478>

[Alexey Proskuryakov]

<rdar://problem/11519288>

[Michael Saboff]

Created attachment 144113 [details]
Patch based on rebasing at 106477 and applying relevant changes to current branch head

This patch is provided as a comparison to the patch I'll provide for review as another way at getting very close to the same answer as the auto/manual back out.

[Michael Saboff]

Created attachment 144114 [details]
Proposed patch

[Michael Saboff]

Created attachment 144116 [details]
Steps I followed to verify that the auto/manual unapply of r106478 and r107345 make sense.

[Filip Pizlo]

Comment on attachment 144114 [details]
Proposed patch

I didn't find any badness from visual inspection of your patch.  Next step is to run some tests.

[Geoffrey Garen]

LGTM.

Looks like we need a proposed patch for TOT and a regression test, as well.

[Andy Wingo]

Apologies for introducing this regression, and for not seeing this bug last week.  I'll take a look at it right now, if you can wait a few hours.

[Andy Wingo]

The following patch fixes paper.js, though it disables an optimization.  Still looking to see what the root fix is.


diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
index fb05e48..dd646c1 100644
--- a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
+++ b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
@@ -1211,7 +1211,7 @@ ResolveResult BytecodeGenerator::resolve(const Identifier& property)
             return ResolveResult::dynamicGlobalResolve(depth, scope);
         return ResolveResult::globalResolve(scope);
     }
-    return ResolveResult::dynamicResolve(depth);
+    return ResolveResult::dynamicResolve(0);
 }
 
 ResolveResult BytecodeGenerator::resolveConstDecl(const Identifier& property)

[Andy Wingo]

Here is a test:


> function foo(x) { with (x) { return function (str) { return eval(str); } } }
undefined
> foo({})("var qux=10; (function () { return qux; })")()
Exception: ReferenceError: Can't find variable: qux

With the patch from my previous comment, it returns qux as it should.  I'll work up a patch with a test case.  Again, apologies for introducing the regression.

[Andy Wingo]

Created attachment 144542 [details]
Patch

[WebKit Review Bot]

Attachment 144542 [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/bytecompiler/BytecodeGenerator.cpp:1213:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Wingo]

Created attachment 144547 [details]
Patch

[Michael Saboff]

Comment on attachment 144547 [details]
Patch

I actually want to check in the reviewed patch https://bugs.webkit.org/attachment.cgi?id=144114.  I will be checking that change into a branch.

Please open a new defect for your proposed changes.

[Andy Wingo]

(In reply to comment #14)
> (From update of attachment 144547 [details])
> I actually want to check in the reviewed patch https://bugs.webkit.org/attachment.cgi?id=144114.  I will be checking that change into a branch.
> 
> Please open a new defect for your proposed changes.

Sorry for obsoleting your patch; didn't intend to do that.  Feel free to use my test if you like.

[Michael Saboff]

Created attachment 144591 [details]
Originally reviewed patch with additional regression test

[Andy Wingo]

Here is Michael's comment from bug 87755 comment 2.  Given its contents, it belongs here.

(From update of attachment 144577 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=144577&action=review

This fix seems a little like a band-aid.  Shouldn't isDynamicScope() return true for scope chain objects that are dynamic and require runtime resolution?  Then the code would break out of the loop at we'd generate the right resolve opcode.

My comment to open a new defect was incorrect.  I think we should close out this defect as no change. We should land the reviewed patch in https://bugs.webkit.org/show_bug.cgi?id=87158 on trunk.  A new defect should be opened or the original refactoring defect should be reopened for your suggested fix or variations.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1173
> +    size_t depthOfFirstScopeWithDynamicChecks = 0;

Why can't the logic for depth be modified so that it provides the right answer?

Shouldn't this variable's name really indicate the depth before the first scope with dynamic checks?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1207
> +                ++depthOfFirstScopeWithDynamicChecks;

If there are multiple scope chain nodes that require dynamic checks, won't this be set to the depth before the last one?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1220
>          if ((flags & ResolveResult::DynamicFlag) && depth)
>              return ResolveResult::dynamicGlobalResolve(depth, scope);

Doesn't this code also need the depth before the first dynamic check?

[Andy Wingo]

(In reply to comment #17)
> 
> This fix seems a little like a band-aid.  Shouldn't isDynamicScope() return true for scope chain objects that are dynamic and require runtime resolution?  Then the code would break out of the loop at we'd generate the right resolve opcode.

There are two kinds of "dynamic" scope objects that can be in the scope chain: activations that use direct non-strict eval, and with objects.  isDynamicScope only returns true for with objects.  It returns false but sets the needsDynamicChecks flag for activations with eval.  In this way the loop can continue, potentially resulting in a op_get_scoped_var, but with a dynamic check.

It's possible to change this, but that particular interface was not changed in my refactoring patch, and so a change to it would be most appropriate as a separate patch.

> My comment to open a new defect was incorrect.  I think we should close out this defect as no change. We should land the reviewed patch in https://bugs.webkit.org/show_bug.cgi?id=87158 on trunk.  A new defect should be opened or the original refactoring defect should be reopened for your suggested fix or variations.

Why do you think a 55 KB rollout is better than a 5 KB bugfix with test case?

I would like ggaren's input, as he reviewed the original patch, and seemed to be happy with it.

> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1173
> > +    size_t depthOfFirstScopeWithDynamicChecks = 0;
> 
> Why can't the logic for depth be modified so that it provides the right answer?

Because the loop needs to track two things: the depth in case of a lexical-with-dynamic-checks resolve result, and the depth of the first dynamic scope for a purely dynamic lookup.

> Shouldn't this variable's name really indicate the depth before the first scope with dynamic checks?

It is a zero-indexed depth of the first scope that has dynamic checks.

> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1207
> > +                ++depthOfFirstScopeWithDynamicChecks;
> 
> If there are multiple scope chain nodes that require dynamic checks, won't this be set to the depth before the last one?

No, because this code is surrounded in a conditional block.

> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1220
> >          if ((flags & ResolveResult::DynamicFlag) && depth)
> >              return ResolveResult::dynamicGlobalResolve(depth, scope);
> 
> Doesn't this code also need the depth before the first dynamic check?

No, because the dynamic global opcode will check intervening dynamic scopes for the identifier.

[Andy Wingo]

I understand if you are trying to stabilize a branch, and you are uncertain about new code from a relatively new contributor.  But I think that applying this rollout to trunk is the wrong thing to do.  The new code is cleaner, more understandable, forms a part of future work to enable let scope, it was reviewed and accepted already, and there is a bugfix available.

[Andy Wingo]

Created attachment 144613 [details]
Patch

[Andy Wingo]

Re-submitted my earlier patch at the suggestion of msaboff on IRC.  The test case needs to cover more resolve cases; I'll get to that tomorrow if review goes well.

[WebKit Review Bot]

Comment on attachment 144613 [details]
Patch

Attachment 144613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12848663

New failing tests:
perf/object-keys.html

[WebKit Review Bot]

Created attachment 144646 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Andy Wingo]

Created attachment 144771 [details]
expanded tests

[Andy Wingo]

Would someone like to review the patch?  msaboff?

[Michael Saboff]

Comment on attachment 144771 [details]
expanded tests

r=me, but the patch seems to trigger another bug.  See https://bugs.webkit.org/show_bug.cgi?id=87994.  There needs to be some coordination of landing this fix with subsequent fix to the new bug.

[Andy Wingo]

Comment on attachment 144771 [details]
expanded tests

Clearing flags on attachment: 144771

Committed r119575: <http://trac.webkit.org/changeset/119575>

[Andy Wingo]

All reviewed patches have been landed.  Closing bug.

[Bastien Dejean]

Still no luck with libwebkit 1.8.3.

[Martin Robinson]

The stable releases are released from a branch. I've added this changeset to the list of proposed merges at https://trac.webkit.org/wiki/WebKitGTK/1.8.x .