Bug 87158 - REGRESSION (r106478): None of the Paper.js JavaScript examples work
Summary: REGRESSION (r106478): None of the Paper.js JavaScript examples work
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Michael Saboff
URL: http://paperjs.org/examples/
Keywords: InRadar, Regression
Depends on: 87994
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-22 12:38 PDT by Bastien Dejean
Modified: 2012-09-01 07:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch based on rebasing at 106477 and applying relevant changes to current branch head (55.29 KB, patch)
2012-05-25 11:50 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Proposed patch (57.34 KB, patch)
2012-05-25 11:50 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Steps I followed to verify that the auto/manual unapply of r106478 and r107345 make sense. (11.07 KB, text/plain)
2012-05-25 11:52 PDT, Michael Saboff
no flags Details
Patch (5.87 KB, patch)
2012-05-29 06:27 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff
Patch (5.87 KB, patch)
2012-05-29 06:55 PDT, Andy Wingo
msaboff: review-
Details | Formatted Diff | Diff
Originally reviewed patch with additional regression test (59.91 KB, patch)
2012-05-29 11:17 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Patch (5.87 KB, patch)
2012-05-29 13:52 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (767.12 KB, application/zip)
2012-05-29 17:08 PDT, WebKit Review Bot
no flags Details
expanded tests (6.71 KB, patch)
2012-05-30 02:43 PDT, Andy Wingo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bastien Dejean 2012-05-22 12:38:33 PDT
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.
Comment 1 Alexey Proskuryakov 2012-05-23 15:19:13 PDT
Confirmed on Mac, regressed in <http://trac.webkit.org/changeset/106478>
Comment 2 Alexey Proskuryakov 2012-05-23 15:20:27 PDT
<rdar://problem/11519288>
Comment 3 Michael Saboff 2012-05-25 11:50:09 PDT
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.
Comment 4 Michael Saboff 2012-05-25 11:50:43 PDT
Created attachment 144114 [details]
Proposed patch
Comment 5 Michael Saboff 2012-05-25 11:52:56 PDT
Created attachment 144116 [details]
Steps I followed to verify that the auto/manual unapply of r106478 and r107345 make sense.
Comment 6 Filip Pizlo 2012-05-25 12:15:39 PDT
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.
Comment 7 Geoffrey Garen 2012-05-25 12:17:12 PDT
LGTM.

Looks like we need a proposed patch for TOT and a regression test, as well.
Comment 8 Andy Wingo 2012-05-29 03:15:53 PDT
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.
Comment 9 Andy Wingo 2012-05-29 05:36:35 PDT
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)
Comment 10 Andy Wingo 2012-05-29 05:48:36 PDT
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.
Comment 11 Andy Wingo 2012-05-29 06:27:16 PDT
Created attachment 144542 [details]
Patch
Comment 12 WebKit Review Bot 2012-05-29 06:32:17 PDT
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.
Comment 13 Andy Wingo 2012-05-29 06:55:50 PDT
Created attachment 144547 [details]
Patch
Comment 14 Michael Saboff 2012-05-29 08:57:06 PDT
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.
Comment 15 Andy Wingo 2012-05-29 09:20:58 PDT
(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.
Comment 16 Michael Saboff 2012-05-29 11:17:56 PDT
Created attachment 144591 [details]
Originally reviewed patch with additional regression test
Comment 17 Andy Wingo 2012-05-29 12:09:18 PDT
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?
Comment 18 Andy Wingo 2012-05-29 12:18:26 PDT
(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.
Comment 19 Andy Wingo 2012-05-29 12:29:01 PDT
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.
Comment 20 Andy Wingo 2012-05-29 13:52:37 PDT
Created attachment 144613 [details]
Patch
Comment 21 Andy Wingo 2012-05-29 13:54:21 PDT
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.
Comment 22 WebKit Review Bot 2012-05-29 17:08:26 PDT
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
Comment 23 WebKit Review Bot 2012-05-29 17:08:30 PDT
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
Comment 24 Andy Wingo 2012-05-30 02:43:32 PDT
Created attachment 144771 [details]
expanded tests
Comment 25 Andy Wingo 2012-05-31 08:44:19 PDT
Would someone like to review the patch?  msaboff?
Comment 26 Michael Saboff 2012-05-31 11:21:07 PDT
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.
Comment 27 Andy Wingo 2012-06-06 02:39:18 PDT
Comment on attachment 144771 [details]
expanded tests

Clearing flags on attachment: 144771

Committed r119575: <http://trac.webkit.org/changeset/119575>
Comment 28 Andy Wingo 2012-06-06 02:39:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Bastien Dejean 2012-09-01 01:51:12 PDT
Still no luck with libwebkit 1.8.3.
Comment 30 Martin Robinson 2012-09-01 07:04:26 PDT
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 .