127557 - getParentMenuButtonRecursive is not defined in linkToolbarItem.js

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Schultz]

i686 Linux from today's CVS:
I get a Javascript exception when following a link in a bugzilla bug list:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "getParentMenuButtonRecursive is not defined"
{file: "chrome://navigator/content/linkToolbarItem.js" line: 87}]' when calling
method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021
(NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

Steps to reproduce:
1) open a bugzilla bug list
2) click on a bug link

I do not see this with a clean profile, but get it everytime with my normal one.
I don't know what component this should be, but Gerv wrote linkToolbarItem.js,
so he gets it.

Comment 1

[Andrew Schultz]

Attachment: patch

move function getParentMenuBottonRecursive outside of function LinkToolbarItem

Comment 2

[Christopher Hoess (gone)]

I'll look at this

Comment 3

[Boris Zbarsky [:bzbarsky]]

To Christopher

Comment 4

[Andrew Schultz]

I see this with binary build 2002022406 but not 2002022208.  possibly a side
effect of bug 62124

Comment 5

[Andrew Schultz]

I mean bug 62164.

Comment 6

[Stuart Ballard]

If it really is a side-effect of bug 62164, Brendan should know about it, as
that patch wasn't supposed to cause any change in behavior ;) Is there any
direct evidence that this bug is caused by that change? Can we reduce this to a
simple JS testcase that fails, and/or verify for sure that bug 62164 was the
cause of this (ie, test on builds identical except for that patch)?

(Brendan, I'm cc'ing you just so that this issue is on your radar; feel free to
take it back off your radar if it seems unrelated to your change, or if you just
aren't interested in it :) )

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I fear it's related, too.  Can someone (timeless?) produce a minimal JS-only
test case that shows us not picking up the function name properly?

Comment 8

[Andrew Schultz]

I also get the error on build 2002022221 (right after checkin of bug 62164)

Comment 9

[Brendan Eich [:brendan]]

It certainly could be that bug 62164 regressed something we don't test for in
the js testsuite; cc'ing pschwartau.  We need a minimal testcase.

/be

Comment 10

[Brendan Eich [:brendan]]

What happens if you quit, remove the FastLoad file from your profile, and
restart?  The comment about it not happening with a clean profile suggests a
problem between FastLoad and the JS engine, although once the clean profile has
been used for one session, there should be a new FastLoad file there, and the
bug should bite again.  Does it?

/be

Comment 11

[Andrew Schultz]

The default profile has the Site Navigation Bar disabled by default.  Enabling
it to "Show Only as Needed" or "Show Always" causes the behavior to appear in a
clean profile.

Comment 12

[Brendan Eich [:brendan]]

I debugged this, it's the second regression known to be caused by the big patch
for bug 62164.  Fix in a moment.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: proposed fix

Amid the massive changes for bug 62164, I made a change to call_enumerate in
jsfun.c of the "one of these things is not like the other -- let's unify!"
sort, and that was not the right thing.  I was comparing how call_enumerate
finds the function object whose properties should be reflected into its call
object with how call_resolve finds the function object in which it will lookup
(including prototype search, as usual) a particular id.

But as the comment in this patch makes clear, call_enumerate is different.  It
must look in the scope of the compiler-created function object, not in a
possibly mutated scope belonging to a cloned function object that shares the
underlying JSFunction and JSScript with its prototype, but that exists to carry
the right scope chain (this arises with nested functions, and in XUL scripts
due to "brutal sharing").

When a XUL script sets .prototype on a function object, it is mutating a clone.
 If call_enumerate looks at that clone's scope, it will see only a property
named 'prototype'.  It will not see parameters and local variables/functions,
so it will miss getParentMenuButtonRecursive in the case at hand in this bug.

I tested this patch with the links toolbar on as needed, it works. 
Confirmation and code review for 0.9.9 sought -- this should go in today. 
Sorry for the trouble, thanks for the good testcase (Phil, we need to cover
this case somehow in the JS suite; I'll find you later today to talk about how
to do that without "brutal sharing").

/be

Comment 14

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 72090 [details] [diff] [review]
proposed fix

When I added function cloning, I had no idea it would be this exciting!

sr=shaver, thanks for the nice long comment, too.

Comment 15

[John Bandhauer]

Comment on attachment 72090 [details] [diff] [review]
proposed fix

r=jband. Looks right to me.

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 72090 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 17

[Brendan Eich [:brendan]]

Fixed in trunk and 0.9.9 branch.

/be

Comment 18

[Christopher Hoess (gone)]

*** Bug 128800 has been marked as a duplicate of this bug. ***

Comment 19

[Phil Schwartau]

Testcase added to JS testsuite:

        mozilla/js/tests/js1_5/Regress/regress-127557.js

This is a testcase that Brendan created, and it depends on the new
clone() function in js.c, the SpiderMonkey shell, just added today:

Usage:
clone     clone(fun[, scope])    Clone function object

Comment 20

[Phil Schwartau]

The testcase now passes in the current JS debug/optimized shell on 
WinNT, Linux, and Mac9.1. However, if the fix for this bug is backed out,
the test fails as follows:

                ReferenceError: h_peer is not defined

Marking Verified FIXED -