unexpected 'arguments' read from the debugger should work like f.arguments, not return 'undefined'

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [firebug-p1])

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

5 years ago
This is breaking some Firebug use cases.
(Assignee)

Updated

5 years ago
Blocks: 740446
(Assignee)

Comment 1

5 years ago
Created attachment 616236 [details] [diff] [review]
fix

So basically the fix is to always add the 'arguments' binding when compiling a script in debug mode.  As the comment explains, this isn't perfect (since you could compile a script when the compartment was not in debug mode) but I think it is good enough for Firebug.  Jan, could you verify that this fix actually fixes your problem?

Jason: this change adds more stuff to a scripts prologue which exposes what I think are bugs in BytecodeRangeWithNumbers/FlowGraphSummary: we want to ignore the prologue so that if 'debugger' is the first line of the script, it is the first offset in the list.  This would avoid observing the script in a semantically-incoherent state.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #616236 - Flags: review?(jorendorff)
Attachment #616236 - Flags: feedback?(odvarko)
(Assignee)

Comment 2

5 years ago
Comment on attachment 616236 [details] [diff] [review]
fix

Actually, this isn't a very good fix: it creates an arguments object for *every* function called in debug mode.  Depending on the code, that could make everything 10x slower.

Jan, how important is it to have frame.arguments?  From the dev-platform post, I see the main use is 'arguments.callee'.  This is disabled in strict-mode, deprecated in non-strict mode, and may eventually get removed altogether, so perhaps this code could be migrated away from 'arguments'?  Are there any other significant uses?

(Note: bug 690135 which in theory could be ready for FF14 will fix this by being able to handle debugger access of scopes separately.)
Attachment #616236 - Flags: review?(jorendorff)
Attachment #616236 - Flags: feedback?(odvarko)
(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 616236 [details] [diff] [review]
> fix
> 
> Actually, this isn't a very good fix: it creates an arguments object for
> *every* function called in debug mode.  Depending on the code, that could
> make everything 10x slower.
> 
> Jan, how important is it to have frame.arguments?  From the dev-platform
> post, I see the main use is 'arguments.callee'.  This is disabled in
> strict-mode, deprecated in non-strict mode, and may eventually get removed
> altogether, so perhaps this code could be migrated away from 'arguments'? 
> Are there any other significant uses?
The watch panel should be able to display 'arguments' when the debugger is halted inside a function call.

Honza
(In reply to Luke Wagner [:luke] from comment #1)
> Created attachment 616236 [details] [diff] [review]
> fix
> 
> So basically the fix is to always add the 'arguments' binding when compiling
> a script in debug mode.  As the comment explains, this isn't perfect (since
> you could compile a script when the compartment was not in debug mode) but I
> think it is good enough for Firebug.  Jan, could you verify that this fix
> actually fixes your problem?
Is try build available somewhere?
Honza
(Assignee)

Comment 5

5 years ago
(In reply to Jan Honza Odvarko from comment #3)
> The watch panel should be able to display 'arguments' when the debugger is
> halted inside a function call.

If 'arguments' is used at all, it will be a regular property of the Call object.
I'm facing the same problem in the Firefox Debugger and this patch fixes it only partially: when the debugger is opened after the page is loaded, I don't get the arguments variable. When I reload the page with the debugger open, I get it, which I guess confirms your comment 1.

STR:

0) enable the debugger by setting devtools.debugger.enabled to true. Set devtools.debugger.log to true as well.
1) apply the WIP patch from bug 724228
2) open the test page browser/devtools/debugger/test/browser_dbg_frame-parameters.html, which in my system has the URL: file:///Users/past/src/fx-team/browser/devtools/debugger/test/browser_dbg_frame-parameters.html
3) open the debugger (Tools -> Web Developer -> Script Debugger)
4) click the 'Click me!' button
5) the newest stack frame in the list has no arguments, unless you resume and reload the page

The "10x slower" note from comment 2 sounds scary, so I think I'll use a workaround that will give me an 'arguments' property without 'callee'.
(Assignee)

Comment 7

5 years ago
Created attachment 618168 [details] [diff] [review]
fix based on bug 690135

As mentioned earlier, here is a fix using the DebugScopeProxy in bug 690135 (which is also currently under review).  This fix has none of the issues in comment 2 or comment 6.  Passes shell debug tests; I still need to test the patch on firebug.
(Assignee)

Comment 8

5 years ago
Created attachment 618471 [details] [diff] [review]
fix arguments (based on bug 690135)

This puts 'arguments' back in the Firebug watch window.
Attachment #618168 - Attachment is obsolete: true
Attachment #618471 - Flags: review?(jimb)
(Assignee)

Comment 9

5 years ago
Created attachment 618506 [details] [diff] [review]
fix arguments (based on bug 690135)

Oops, forgot to run jit_tests.py; turns out this patch fixes some more existing debugger tests.
Attachment #618471 - Attachment is obsolete: true
Attachment #618471 - Flags: review?(jimb)
Attachment #618506 - Flags: review?(jimb)
This report is blocking me from releasing Firebug (labeling as p1)

Jim, could you please review the patch? :-)

Honza
Whiteboard: [firebug-p1]
Also note that I can see the problem also in Aurora
(Built from http://hg.mozilla.org/releases/mozilla-aurora/rev/8a68244c078b).

Honza

Comment 12

5 years ago
(In reply to Jan Honza Odvarko from comment #10)
> Jim, could you please review the patch? :-)

It's my priority today, along with bug 690135, on which it depends.
yay!

Honza

Comment 14

5 years ago
I've gotten 690135 reviewed today. I'll take on this one next, but I'm going to knock off for today.
(Assignee)

Updated

5 years ago
Depends on: 752770
(Assignee)

Comment 15

5 years ago
Honza: here is a build with all the new debug scope proxies (bug x) and the patch in this bug:

  https://tbpl.mozilla.org/?tree=Try&rev=819213e6aecc

I did a basic Firebug trial run and the scope stuff seems to work fine.  Could you confirm?
(In reply to Luke Wagner [:luke] from comment #15)
> I did a basic Firebug trial run and the scope stuff seems to work fine. 
> Could you confirm?
Yep, I can confirm, both my test cases work now, thanks!

Just a note, Firebug related issue:
http://code.google.com/p/fbug/issues/detail?id=5407

Honza
(Assignee)

Comment 17

5 years ago
(In reply to Jan Honza Odvarko from comment #16)
Thanks!  In general, does Firebug seem non-broken with that build?  (I ask b/c the build contains non-trivial changes for the debugger support implementation.)
(In reply to Luke Wagner [:luke] from comment #17)
> (In reply to Jan Honza Odvarko from comment #16)
> Thanks!  In general, does Firebug seem non-broken with that build?  (I ask
> b/c the build contains non-trivial changes for the debugger support
> implementation.)
There is one thing.

STR:
1) Install Firebug (run from source: git@github.com:firebug/firebug.git, see instruction how to run directly from source: https://github.com/firebug/firebug/blob/master/README.md)

2) Launch Firefox open one tab e.g.: www.google.com
3) Press F12 to open Firebug UI select and enable the Script panel, click reload. You should see some source code in the panel
4) Open a new tab and switch back to www.google.com
5) Now the Script panel says: Debugger not activated

I am seeing an exception in FBTrace console:
Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [jsdIDebuggerService.unPause]

resource://firebug/firebug-service.js, line 1842

the line calls: jsd.unPause();

https://github.com/firebug/firebug/blob/master/extension/modules/firebug-service.js#L1842

I don's see the exception in latest Nightly

Honza
(Assignee)

Comment 19

5 years ago
I think this bug was caused by a limitation I added and (coincidentally) just removed today: I triggered a failure if you tried to turn debug mode off while the script was active (which is what I think is happening when you switch tabs?).  With this fix, my Script panel works after switching tabs.  I'll put up a new try build soon.  Thanks again!
(Assignee)

Comment 20

5 years ago
Here's the new build:
https://tbpl.mozilla.org/?tree=Try&rev=0d106223dc4a
(In reply to Luke Wagner [:luke] from comment #20)
> Here's the new build:
> https://tbpl.mozilla.org/?tree=Try&rev=0d106223dc4a
Yep this build fixes the problem described in comment 18 and also both my original test cases work, thanks!

Honza
(Assignee)

Comment 22

5 years ago
Created attachment 622832 [details] [diff] [review]
fix arguments (based on bug 690135)

Rebased
Attachment #618506 - Attachment is obsolete: true
Attachment #618506 - Flags: review?(jimb)
Attachment #622832 - Flags: review?(jimb)

Comment 23

5 years ago
Comment on attachment 622832 [details] [diff] [review]
fix arguments (based on bug 690135)

Review of attachment 622832 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/debug/Frame-eval-08.js
@@ +11,1 @@
>  };

I like this test; it would be nice to also have a variant in which f calls a function g, g does a 'debugger;' statement, and the onDebuggerStatement handler uses frame.older.eval("arguments") to get the arguments from a lightweight frame.

@@ +15,5 @@
> +        assertEq(args[0], this);
> +        assertEq(args[1], f);
> +        assertEq(args[2].toString(), "[object Object]");
> +        return 42;
> +    } + ";");

Couldn't we just write these assertions out at top level, after calling f? assertEq(args[0], this) would become assertEq(args[0], g)...

::: js/src/vm/ScopeObject.cpp
@@ -309,5 @@
>          *vp = callobj.var(i);
>  
> -    /* This can only happen via the debugger. Bug 659577 will remove it. */
> -    if (vp->isMagic(JS_OPTIMIZED_ARGUMENTS))
> -        *vp = UndefinedValue();

Could we replace this with an assertion that we didn't fetch a JS_OPTIMIZED_ARGUMENTS magic value?

@@ +1272,5 @@
> +        return scope.isCall() && !scope.asCall().isForEval() &&
> +               !scope.asCall().getCalleeFunction()->script()->argumentsHasLocalBinding();
> +    }
> +
> +    static ArgumentsObject *getMissingArguments(JSContext *cx, jsid id, ScopeObject &scope)

This function doesn't just 'get' missing arguments objects, it checks whether that's what's being requested. Woul 'checkForMissingArguments' be better?

It would be nice to have a one- or two-line comment above the function.

@@ +1283,5 @@
> +            return NULL;
> +
> +        StackFrame *fp = scope.maybeStackFrame();
> +        if (!fp)
> +            return NULL;

The way this function uses the 'NULL' return value makes me uncomfortable.

At the top, returning NULL means, "This isn't a request for arguments that might need to be faked." But the last return here seems to mean, "I'm unable to produce an arguments object for this CallObject because its stack frame is gone. Given the way it's used, I'd expect getMissingArguments to always return non-NULL for a CallObject that one would expect to have an arguments object, regardless of whether its associated stack frame still exists.

In other words, calling find('arguments') on a Debugger.Environment representing a popped call frame shouldn't scan outwards to enclosing frames --- but if I'm reading right, it will because we return NULL here.
(Assignee)

Comment 24

5 years ago
(In reply to Jim Blandy :jimb from comment #23)
> Couldn't we just write these assertions out at top level, after calling f?
> assertEq(args[0], this) would become assertEq(args[0], g)...

I was basically returning this test to its previous state before bug 740446.

> Could we replace this with an assertion that we didn't fetch a
> JS_OPTIMIZED_ARGUMENTS magic value?

Sure.  I didn't bother since the whole getter is removed in bug 690135.

> In other words, calling find('arguments') on a Debugger.Environment
> representing a popped call frame shouldn't scan outwards to enclosing frames
> --- but if I'm reading right, it will because we return NULL here.

Ah, good point.  I'll have it throw JSMSG_DEBUG_NOT_LIVE.
(Assignee)

Comment 25

5 years ago
Created attachment 624458 [details] [diff] [review]
fix arguments

With comments addressed and new test for the 'not live' case.
Attachment #616236 - Attachment is obsolete: true
Attachment #622832 - Attachment is obsolete: true
Attachment #622832 - Flags: review?(jimb)
Attachment #624458 - Flags: review?(jimb)
(Assignee)

Comment 26

5 years ago
Created attachment 624534 [details] [diff] [review]
fix arguments

Pre-emptive irc nit-fix!!

On second and third reflection, this interface for checkForMissingArguments is much simpler; good suggestion.
Attachment #624458 - Attachment is obsolete: true
Attachment #624458 - Flags: review?(jimb)
Attachment #624534 - Flags: review?(jimb)
(Assignee)

Comment 27

5 years ago
Created attachment 624620 [details] [diff] [review]
fix arguments

One last thing: this patch overrides ProxyHandler::has (so the default doesn't need to call getPropertyDescriptor and create an args obj).
Attachment #624620 - Flags: review?(jimb)
(Assignee)

Updated

5 years ago
Attachment #624534 - Attachment is obsolete: true
Attachment #624534 - Flags: review?(jimb)

Comment 28

5 years ago
(In reply to Luke Wagner [:luke] from comment #26)
> On second and third reflection, this interface for checkForMissingArguments
> is much simpler; good suggestion.

I'm glad you feel that way! I was afraid I was going to be the world's first nit-aggravated murder victim. :)
(Assignee)

Comment 29

5 years ago
Indeed.  (Btw, this bug and bug 690135 are ready to land (ahead of bug 659577) as soon as you finish the review.)
(Assignee)

Comment 30

5 years ago
Created attachment 625140 [details] [diff] [review]
fix arguments

... and of course my quick addition of ProxyHandler::has was completely bogus; this one actually passes tests.
Attachment #624620 - Attachment is obsolete: true
Attachment #624620 - Flags: review?(jimb)
Attachment #625140 - Flags: review?(jimb)

Comment 31

5 years ago
It seems like bug 740446 also munged jit-test/tests/debug/Frame-evalWithBindings-03.js and Frame-onPop-21.js; could those be restored, too?
(Assignee)

Comment 32

5 years ago
Yes

Comment 33

5 years ago
Comment on attachment 625140 [details] [diff] [review]
fix arguments

Review of attachment 625140 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great; r=me with those other tests restored.

::: js/src/vm/ScopeObject.cpp
@@ +1314,5 @@
> +
> +        StackFrame *fp = scope.maybeStackFrame();
> +        if (!fp) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_DEBUG_NOT_LIVE,
> +                                 "Debugger scope");

I think in the future we're going to need a generic "Unfortunately, this variable that you would expect to be present isn't actually available" message. But this is fine for now.
Attachment #625140 - Flags: review?(jimb) → review+
(Assignee)

Comment 34

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d21a80aeef05
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/d21a80aeef05
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Any chance the patch is ported into Firefox 14?
Honza
(Assignee)

Comment 37

5 years ago
This fix landed in FF15 depends on another large FF15 patch.  A fix in FF14 would require a completely custom FF14 patch along the lines of comment 1 (which would make every function call a lot slower (and produce a lot more GCs) when in debug mode).
OK, I see, thanks for the update Luke!
Honza
You need to log in before you can comment on or make changes to this bug.