Last Comment Bug 746601 - unexpected 'arguments' read from the debugger should work like f.arguments, not return 'undefined'
: unexpected 'arguments' read from the debugger should work like f.arguments, n...
Status: RESOLVED FIXED
[firebug-p1]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 752770
Blocks: 740446
  Show dependency treegraph
 
Reported: 2012-04-18 09:07 PDT by Luke Wagner [:luke]
Modified: 2012-06-15 09:04 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (11.27 KB, patch)
2012-04-18 12:10 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix based on bug 690135 (5.54 KB, patch)
2012-04-24 22:22 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix arguments (based on bug 690135) (7.39 KB, patch)
2012-04-25 16:00 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix arguments (based on bug 690135) (9.32 KB, patch)
2012-04-25 18:45 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix arguments (based on bug 690135) (9.16 KB, patch)
2012-05-10 12:02 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix arguments (11.88 KB, patch)
2012-05-16 11:16 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix arguments (11.67 KB, patch)
2012-05-16 14:19 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix arguments (11.97 KB, patch)
2012-05-16 18:48 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
fix arguments (12.25 KB, patch)
2012-05-18 09:33 PDT, Luke Wagner [:luke]
jimb: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-04-18 09:07:53 PDT
This is breaking some Firebug use cases.
Comment 1 Luke Wagner [:luke] 2012-04-18 12:10:31 PDT
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.
Comment 2 Luke Wagner [:luke] 2012-04-18 12:48:17 PDT
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.)
Comment 3 Jan Honza Odvarko [:Honza] 2012-04-19 07:28:27 PDT
(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
Comment 4 Jan Honza Odvarko [:Honza] 2012-04-19 07:38:35 PDT
(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
Comment 5 Luke Wagner [:luke] 2012-04-19 09:13:35 PDT
(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.
Comment 6 Panos Astithas [:past] 2012-04-20 07:56:05 PDT
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'.
Comment 7 Luke Wagner [:luke] 2012-04-24 22:22:31 PDT
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.
Comment 8 Luke Wagner [:luke] 2012-04-25 16:00:18 PDT
Created attachment 618471 [details] [diff] [review]
fix arguments (based on bug 690135)

This puts 'arguments' back in the Firebug watch window.
Comment 9 Luke Wagner [:luke] 2012-04-25 18:45:39 PDT
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.
Comment 10 Jan Honza Odvarko [:Honza] 2012-05-04 06:25:14 PDT
This report is blocking me from releasing Firebug (labeling as p1)

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

Honza
Comment 11 Jan Honza Odvarko [:Honza] 2012-05-04 07:46:21 PDT
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 Jim Blandy :jimb 2012-05-04 08:53:07 PDT
(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.
Comment 13 Jan Honza Odvarko [:Honza] 2012-05-04 08:55:58 PDT
yay!

Honza
Comment 14 Jim Blandy :jimb 2012-05-04 17:26:21 PDT
I've gotten 690135 reviewed today. I'll take on this one next, but I'm going to knock off for today.
Comment 15 Luke Wagner [:luke] 2012-05-07 21:09:17 PDT
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?
Comment 16 Jan Honza Odvarko [:Honza] 2012-05-08 05:46:33 PDT
(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
Comment 17 Luke Wagner [:luke] 2012-05-08 11:15:00 PDT
(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.)
Comment 18 Jan Honza Odvarko [:Honza] 2012-05-09 03:05:45 PDT
(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
Comment 19 Luke Wagner [:luke] 2012-05-09 12:29:46 PDT
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!
Comment 20 Luke Wagner [:luke] 2012-05-09 13:54:06 PDT
Here's the new build:
https://tbpl.mozilla.org/?tree=Try&rev=0d106223dc4a
Comment 21 Jan Honza Odvarko [:Honza] 2012-05-10 05:03:26 PDT
(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
Comment 22 Luke Wagner [:luke] 2012-05-10 12:02:49 PDT
Created attachment 622832 [details] [diff] [review]
fix arguments (based on bug 690135)

Rebased
Comment 23 Jim Blandy :jimb 2012-05-15 21:09:00 PDT
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.
Comment 24 Luke Wagner [:luke] 2012-05-16 09:53:21 PDT
(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.
Comment 25 Luke Wagner [:luke] 2012-05-16 11:16:55 PDT
Created attachment 624458 [details] [diff] [review]
fix arguments

With comments addressed and new test for the 'not live' case.
Comment 26 Luke Wagner [:luke] 2012-05-16 14:19:20 PDT
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.
Comment 27 Luke Wagner [:luke] 2012-05-16 18:48:42 PDT
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).
Comment 28 Jim Blandy :jimb 2012-05-17 16:19:38 PDT
(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. :)
Comment 29 Luke Wagner [:luke] 2012-05-17 19:52:45 PDT
Indeed.  (Btw, this bug and bug 690135 are ready to land (ahead of bug 659577) as soon as you finish the review.)
Comment 30 Luke Wagner [:luke] 2012-05-18 09:33:29 PDT
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.
Comment 31 Jim Blandy :jimb 2012-05-18 10:36:48 PDT
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?
Comment 32 Luke Wagner [:luke] 2012-05-18 10:52:09 PDT
Yes
Comment 33 Jim Blandy :jimb 2012-05-18 11:46:09 PDT
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.
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-05-18 18:41:34 PDT
https://hg.mozilla.org/mozilla-central/rev/d21a80aeef05
Comment 36 Jan Honza Odvarko [:Honza] 2012-06-15 03:11:02 PDT
Any chance the patch is ported into Firefox 14?
Honza
Comment 37 Luke Wagner [:luke] 2012-06-15 09:02:44 PDT
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).
Comment 38 Jan Honza Odvarko [:Honza] 2012-06-15 09:04:49 PDT
OK, I see, thanks for the update Luke!
Honza

Note You need to log in before you can comment on or make changes to this bug.