Last Comment Bug 760071 - Debug scope proxies don't let eval-in-frame code define new properties
: Debug scope proxies don't let eval-in-frame code define new properties
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla15
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-31 05:19 PDT by Jan Honza Odvarko [:Honza]
Modified: 2012-06-04 12:14 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix and tests (2.61 KB, patch)
2012-05-31 12:07 PDT, Luke Wagner [:luke]
jimb: review+
Details | Diff | Review

Description Jan Honza Odvarko [:Honza] 2012-05-31 05:19:10 PDT
Firebug's test doesn't work since the "TypeError: can't redefine non-configurable property" error appears when evaluating the following script in a frame:

if (!window._firebug)window._firebug={};
window._firebug.rerunThis = this;
window._firebug.rerunArgs = [];
if (arguments && arguments.length) for (var i = 0; i < arguments.length; i++) window._firebug.rerunArgs.push(arguments[i]);
window._firebug.rerunFunctionName = onclick;
window._firebug.rerunFunction = function _firebugRerun() { onclick.apply(window._firebug.rerunThis, window._firebug.rerunArgs); }

See the source here:
https://github.com/firebug/firebug/blob/master/extension/content/firebug/js/debugger.js#L445

STR:
1) Install Firebug: http://getfirebug.com/releases/firebug/1.10/firebug-1.10.0a9.xpi
2) Load: http://getfirebug.com/tests/head/script/callstack/3645/issue3645.html
3) Follow the steps on the page -> after clicking re-run, the debugger doesn't halt again -> BUG

You can also see the error by installing FBTrace: 
http://getfirebug.com/releases/fbtrace/1.10/fbTrace-1.10b1.xpi

Open FBTrace by "Firebug (icon) menu -> Open Firebug Tracing". In the console check UI_LOOP option (within the Options tab) and watch the console when pressing the rerun button during the test case.

The log comes from this source location:
https://github.com/firebug/firebug/blob/master/extension/content/firebug/js/debugger.js#L468

The log:
debugger.rerun false and result: [object Object] for http://legoas/src/github.com/firebug/firebug/tests/content/script/callstack/3645/issue3645.html

- Expand the log
- Select the Object tab
- Result -> Value -> stringValue == "TypeError: can't redefine non-configurable property 'i'"

It works in Firefox 13

Is there something wrong with the evaluated script?

Honza
Comment 1 Luke Wagner [:luke] 2012-05-31 11:00:52 PDT
That error pops out when eval-in-frame code tries to redefine a binding in the enclosing scope.  Right now, the error is overly conservative (it throws on *any* call to defineProperty on the scope object) since it wasn't even clear how this path would be exercised (normal reads/writes go through get/set).  I'll look at what your test-case is doing and probably we can relax the restriction to allow defineProperty when, e.g., only the value changes.
Comment 2 Luke Wagner [:luke] 2012-05-31 12:07:20 PDT
Created attachment 628845 [details] [diff] [review]
fix and tests

Simple enough fix; thanks for the great STR!  With this patch, do you see any other problems Jan?

The eif-call-newvar change is just reverting the change made in bug 690135.
Comment 3 Jim Blandy :jimb 2012-05-31 13:13:59 PDT
Comment on attachment 628845 [details] [diff] [review]
fix and tests

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

Looks good to me. Would it be worthwhile to test that a *re*declaration of a variable also works?
Comment 4 Luke Wagner [:luke] 2012-05-31 14:01:50 PDT
(In reply to Jim Blandy :jimb from comment #3)
Heh, well, that actually doesn't work with the patch (that's what the if(has) Throw is guarding against).  To make it work, we'd need to do a lot more thinking to make sure the PropertyDescriptors match and then the soon-to-be-landed patches in bug 659577 would need to do the right thing for unaliased variables.  All in all, I'd rather not do it if it isn't necessary.
Comment 5 Luke Wagner [:luke] 2012-05-31 17:50:58 PDT
As jimb pointed out on irc, re-defining a var does not call defineProperty, but just sets the existing var, so it should work just fine.  Pushed with the suggested redefinition test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b99d0e6e281
Comment 6 Ed Morley [:emorley] 2012-06-01 08:09:04 PDT
https://hg.mozilla.org/mozilla-central/rev/6b99d0e6e281
Comment 7 Jan Honza Odvarko [:Honza] 2012-06-01 09:09:31 PDT
(In reply to Luke Wagner [:luke] from comment #2)
> Simple enough fix; thanks for the great STR!  With this patch, do you see
> any other problems Jan?
It looks like this will be in Nightly soon so, I'll let you know.

Thanks for the quick fix!

Honza
Comment 8 Jan Honza Odvarko [:Honza] 2012-06-04 12:14:19 PDT
Retested with today's Nightly and STR from comment #0 works for me

Thanks!
Honza

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