702572 - Minimized prototype.js broken in Firefox 9+

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Lucien]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Build ID: 20111109112850

Steps to reproduce:

Video of the bug : http://screencast.com/t/gsgffQZM

URL : http://www.ajaxplorer.info/demo/


Actual results:

This function is existing, and not existing :/


Expected results:

This page worked till the last Firefox 9 beta version of this morning.

Function ... worked. Now it says it's not existing, but it is.

Comment 1

[Lucien]

I unpacked the js and now it works...

Comment 2

[Jan de Mooij [:jandem]]

I can confirm the "Form.serialize is not a function" message in Nightly builds, even if I add the JS to a simple HTML file. We should understand what's happening here before this reaches the stable channel.

Do you know what minimizer was used here?

Comment 3

[Jan de Mooij [:jandem]]

This regressed on the Aurora branch. Still bisecting...

Comment 4

[Jan de Mooij [:jandem]]

11/01 WORKS - http://hg.mozilla.org/releases/mozilla-aurora/rev/c8fcdb6bd4d7
11/02 FAILS - http://hg.mozilla.org/releases/mozilla-aurora/rev/90a4c98c1ae3

http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=c8fcdb6bd4d7&tochange=90a4c98c1ae3

Almost certainly bug 684489.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Testcase

Standalone HTML file. Shows "Form.serialize is not a function" in the web console in Firefox 9+.

Lucien, thanks for the bug report!

Comment 6

[Jan de Mooij [:jandem]]

Anybody willing to look into this? It definitely seems to be a regression, and the combination minimizerX + prototype.js may be very common...

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: unpacked testcase, still triggers the error

Comment 8

[Jason Orendorff [:jorendorff]]

Shell testcase:

  eval("function f() { function g() {} return g; }");
  assertEq(f().prototype !== f().prototype, true);

Comment 9

[Jason Orendorff [:jorendorff]]

Related:

function f() {
    function g() {
        function h() { return h; }
    }
    return g;
}
assertEq(f().prototype !== f().prototype, true);


The first bad revision is:
changeset:   37685:36bbd730e24f
user:        Brendan Eich <[e-mail]>
date:        Thu Jan 14 09:33:14 2010 -0800
summary:     Analyze module pattern and private-statics pattern in order to despecialize from methods to slots/sprops (536564, r=jorendorff).

Comment 10

[David Mandelin [:dmandelin]]

Attachment: Strawman patch

This is a bad bug for the browser (as is the hopefully dup bug 706972), and I want to get it fixed for 9 if release-drivers will take it. 

The attached strawman patch fixes the test case in comment 8 and passes shell tests. I think it might work, suitably cleaned up. Explanation of what I found:

In the pre-regression code, in SetFunctionKinds we go to the final case and g ends up getting optimized to a null closure. This is AIUI, wrong. But continuing from there, in JSOP_DEFLOCALFUN, being a null closure takes us into the first arm of the if, which always clones. The functions in the test case thus are unequal, and the assertion passes.

In the post-regression code, in SetFunctionKinds we take the 2nd case (with newly weakened condition) and don't optimize. Then in JSOP_DEFLOCALFUN, we take the second arm of the if, which optimizes by cloning only if the function parent is different. I believe that in this case, because we are using DEFLOCALFUN and g doesn't use any variables defined in f, f is not marked heavyweight, so it doesn't get a call object. Thus, both copies of g get parented to the call object of the eval (I think, didn't check that, but if not, it would be the global object). So the optimization applies, and we don't clone. The result is the functions come out identical, and the assertion fails.

I'm not sure where the bug actually is (and there are probably multiple answers to that question), but I figure that when we don't clone, a read barrier is supposed to fire at some point to clone if needed. I think those are only on properties, so they will not apply to the object created by DEFLOCALFUN, which is in a local variables. I would vaguely guess that the optimization in DEFLOCALFUN is just wrong, but was masked by the regressing bug.

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 12

[Alex Keybl [:akeybl]]

We've gotten a couple of 100% CPU reports caused by this bug (706911 and 706972). We're more likely to take this in FF9 if reviewed and landable tomorrow before we go to build for the second to last FF9 beta.

Please make sure to include a risk assessment when nominating. Thanks!

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 578732 [details] [diff] [review]
Patch

Excellent. I love it. Methodjit needs to be updated too though.

Comment 14

[David Mandelin [:dmandelin]]

Comment on attachment 578732 [details] [diff] [review]
Patch

Jason pointed out on IRC that we also need to update DefLocalFun in the JM stubs.

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Patch v2

With methodjit change. Also, I realized that v1 had removed a null check on the output of GetScopeChainFast, so I fixed that.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 579391 [details] [diff] [review]
Patch v2


>-        obj = CloneFunctionObjectIfNotSingleton(f.cx, fun, &f.fp()->scopeChain());
>-        if (!obj)
>-            THROWV(NULL);
>+        parent = &f.regs.fp()->scopeChain();
>     } else {
>-        JSObject *parent = GetScopeChainFast(f.cx, f.fp(), JSOP_DEFLOCALFUN,
>-                                             JSOP_DEFLOCALFUN_LENGTH);
>+        parent = GetScopeChainFast(f.cx, f.regs.fp(), JSOP_DEFLOCALFUN,
>+                                   JSOP_DEFLOCALFUN_LENGTH);

In both places f.regs.fp() could just be written f.fp().

r=me either way.

Comment 17

[Christopher Blizzard (:blizzard)]

Is this somewhat like bug 694454?  We keep finding bugs with prototype & walking up the scope chain.  Do we not have good test cases here?  Are they related?

Comment 18

[David Mandelin [:dmandelin]]

(In reply to Christopher Blizzard (:blizzard) from comment #17)
> Is this somewhat like bug 694454?  We keep finding bugs with prototype &
> walking up the scope chain.  Do we not have good test cases here?  Are they
> related?

Luke has already started an overhaul of scope chains--things should be simpler and more reliable when he is done.

Comment 19

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bab6fef4017c

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bab6fef4017c

Comment 21

[David Mandelin [:dmandelin]]

Comment on attachment 579391 [details] [diff] [review]
Patch v2

Nominating for landing to Aurora and Beta. Benefits:

- Breaking minimized prototype.js could break a lot of sites, so fixing it is good.
- We already have another site that hangs (bug 706911) due to this bug and is fixed by this patch.
- We could consider backing out the regressing patch (the one that caused this bug), but that patch fixes a regression in Google Maps, so that's not very appealing.

Risks are low:

- A small risk is that some site out there depends on the old buggy behavior and will be broken by the new behavior.
- The patch is only about 20 lines of code, and it's mostly 2 parts duplicated, so really only 10 lines to think about.
- The effect of the patch is just to disable an optimization that rarely comes up in practice (so the expected impact on performance is negligible).

Comment 22

[Alex Keybl [:akeybl]]

Comment on attachment 579391 [details] [diff] [review]
Patch v2

[Triage Comment]
Approving for Aurora since we'd want to get testing there before considering for the next beta build at this point.

We'll start weighing the options for beta (taking this vs backing out) at tomorrow's channel meeting.

Comment 24

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/9d149d145f6c

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 579391 [details] [diff] [review]
Patch v2

Approved for beta per todays drivers meeting. While this is not a trivial fix, it feels better to take this than to back out the patch that caused this.

Comment 26

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-beta/rev/77d42322277d

Comment 27

[u279076]

(In reply to David Mandelin from comment #26)
> http://hg.mozilla.org/releases/mozilla-beta/rev/77d42322277d

A note to QA testers, this fix will not be verifiable until we have 9.0b6 builds.

Comment 28

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111214 Firefox/10.0a2

Verified the fix on the above builds and issue is no longer reproducible: web console des not return "Form.serialize() is not a function" anymore

Comment 29

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111214 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111214 Firefox/11.0a1

Verified the fix on the above builds and issue is no longer reproducible: web console des not return "Form.serialize() is not a function" anymore

Comment 30

[Lucien]

Thanks a lot ;)