Closed Bug 998908 Opened 10 years ago Closed 10 years ago

[jsdbg2] Calling Debugger.Script.prototype.getChildScripts causes errors to be thrown that otherwise wouldn't be

Categories

(Core :: JavaScript Engine, defect)

30 Branch
x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox30 --- wontfix
firefox31 --- verified
firefox32 --- verified
relnote-firefox --- 30+

People

(Reporter: andershol, Assigned: shu)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [firebug-p1] )

Attachments

(2 files)

To reproduce:
- Open a page that uses the google maps api (v3). This is the most basic example from googles api documentation:
  https://google-developers.appspot.com/maps/documentation/javascript/examples/full/map-simple
- Confirm that the map is showing correctly
- Open the developer tools debugger (the problem also occurs using firebug)
- Reload the page and observe that the map do not appear. The console have the error: "ReferenceError: pd is not defined"
(closing the developer tools and reloading makes the map appear)
This only seems to happen when the debugger is open; if I try to repro w/ console or inspector the map loads.
Status: UNCONFIRMED → NEW
Ever confirmed: true
What I see is, as I said, that the problem appears when the debugger tab is opened, but doesn't disappear again until the entire developer tools panel is closed. That is, it is not enough to switch away form the debugger tab.

Last good revision: 0f472b5d1d7a
First bad revision: 8441e0585107
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0f472b5d1d7a&tochange=8441e0585107
Interesting, the only debugger related commit is:

63fb1f3f6b0b	Nick Fitzgerald — Bug 978019 - Fix breakpoints after reload on immediately invoked function expressions; r=past

Seeing if reverting that commit fixes the problem.
Yup, bug 978019 introduced this. No idea why though... this seems insane to me. Thanks for bisecting!
Depends on: 978019
Other test cases (from bug 996456):

* http://docs.sencha.com/extjs/4.2.2/extjs-build/examples/feed-viewer/feed-viewer.html - Whole page fails to load with the debugger open.

* http://www.ariatemplates.com/ - Carousel in the top left fails to load with the debugger open.
This is very perplexing to me, because the only substantial change in the patch that caused this regression is:

   onNewScript: function (aScript, aGlobal) {
     this._addScript(aScript);
+
+    // |onNewScript| is only fired for top level scripts (AKA staticLevel == 0),
+    // so we have to make sure to call |_addScript| on every child script as
+    // well to restore breakpoints in those scripts.
+    for (let s of aScript.getChildScripts()) {
+      this._addScript(s);
+    }
+
     this.sources.sourcesForScript(aScript);
   },

All it does is make sure breakpoints are added in child scripts. The |_addScript| method doesn't do anything else.
(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> This is very perplexing to me, because the only substantial change in the
> patch that caused this regression is:
> 
>    onNewScript: function (aScript, aGlobal) {
>      this._addScript(aScript);
> +
> +    // |onNewScript| is only fired for top level scripts (AKA staticLevel
> == 0),
> +    // so we have to make sure to call |_addScript| on every child script as
> +    // well to restore breakpoints in those scripts.
> +    for (let s of aScript.getChildScripts()) {
> +      this._addScript(s);
> +    }
> +
>      this.sources.sourcesForScript(aScript);
>    },
> 
> All it does is make sure breakpoints are added in child scripts. The
> |_addScript| method doesn't do anything else.

So is it definitely that loop triggering the problem - if you remove the call to this._addScript(s) the problem goes away?
Testing right now whether it is the |_addScript| call or the |getChildScripts| call...
Even without calling |_addScript| I can reproduce the failure (d.el is not defined on the sencha feed page):

   onNewScript: function (aScript, aGlobal) {
     this._addScript(aScript);
 
     // |onNewScript| is only fired for top level scripts (AKA staticLevel == 0),
     // so we have to make sure to call |_addScript| on every child script as
     // well to restore breakpoints in those scripts.
     for (let s of aScript.getChildScripts()) {
-      this._addScript(s);
+      // this._addScript(s);
     }
 
     this.sources.sourcesForScript(aScript);
   },

When I comment out the whole loop and call to |getChildScripts| everything works. It seems that calling |getChildScripts| is what triggers the failure.
Maybe the fact that |getChildScripts| ensures there's an actual |JSScript| (ie not lazy) for every function has something to do with this? But I thought we already did that when entering debug mode...
(In reply to Jan de Mooij [:jandem] from Bug 996456 comment #1)
> I can repro this on OS X.
> 
> Also fails with javascript.options.baselinejit = false

Maybe this is just a bug in baseline that we run into when we go into debug mode (and therefore baseline)?
(In reply to Nick Fitzgerald [:fitzgen] from comment #12)
> (In reply to Jan de Mooij [:jandem] from Bug 996456 comment #1)
> > I can repro this on OS X.
> > 
> > Also fails with javascript.options.baselinejit = false
> 
> Maybe this is just a bug in baseline that we run into when we go into debug
> mode (and therefore baseline)?

Hrm, no I can't repro with that pref as false.
Component: Developer Tools: Debugger → JavaScript Engine
Product: Firefox → Core
Summary: ReferenceError in google maps when developer tools debugger is active → [jsdbg2] Calling Debugger.Script.prototype.getChildScripts causes errors to be thrown that otherwise wouldn't be
Whiteboard: [firebug-p1]
Blocks: 978019
No longer depends on: 978019
I've been looking at the extjs feed-viewer case and I haven't really made any progress.

What's going on is that when running under the debugger, somehow a div element with the id #toolbar-1012 doesn't get created (in fact, the DOM is different with the debugger open), and the code is failing because document.getElementById('toolbar-1012') returns null.

I have no idea why this happens.
Does anything in devtools intercept DOM calls and might mess with the dynamically created DOM?
Attached file test.html
After 5 hours of debugging I got a reduced test case.

Direct eval is behaving differently with the debugger on. If you run the test page with the debugger on, it'll error out on 'obj' being undefined.
$'s call obj without debugger on:

(gdb) p js_DumpObject(parent.get())
object 0x7fffdf91a3a8
class 0x7ffff63cf8d0 Call
flags: delegate varobj
proto null
parent null
reserved slots:
   0 (reserved) = <Window object at 0x7fffd4fe1060>
   1 (reserved) = <function f (file:///home/shu/test.html:3) at 0x7fffd41fbbc0>
properties:
    ((JSShape *) 0x7fffd4100100) enumerate permanent JSString* (0x7fffdef004a0) = jschar * (0x7fffdef004b0) = "$"
: slot 2 = undefined
    ((JSShape *) 0x7fffd4100128) enumerate permanent JSString* (0x7fffdef24060) = jschar * (0x7fffdef24070) = "arguments"
: slot 3 = <invalid ?!>
    ((JSShape *) 0x7fffd41001a0) enumerate JSString* (0x7fffdef297e0) = jschar * (0x7fffdef297f0) = "obj"
: slot 4 = <Object at 0x7fffd4ff0b50>

With debugger on:

(gdb) p js_DumpObject(parent.get())
object 0x7fffdf903460
class 0x7ffff63cf8d0 Call
flags: delegate varobj
proto null
parent null
reserved slots:
   0 (reserved) = <Sandbox object at 0x7fffd0dc72e0>
   1 (reserved) = <function defer (resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/deprecated-sync-thenables.js:31) at 0x7fffd8a45dc0>
properties:
    ((JSShape *) 0x7fffd3a46150) enumerate permanent JSString* (0x7fffdef55d08) = jschar * (0x7fffdef55d18) = "observers"
: slot 2 = <Array object at 0x7fffdf9034c0>
    ((JSShape *) 0x7fffd3a46330) enumerate permanent JSString* (0x7fffdef2b060) = jschar * (0x7fffdef2b070) = "result"
: slot 3 = null
    ((JSShape *) 0x7fffd3a46358) enumerate permanent JSString* (0x7fffdef556f0) = jschar * (0x7fffdef55700) = "deferred"
: slot 4 = undefined

Someone tell me what's up with this?
Closing but, because it seems like we're leaking a chrome global.
Group: javascript-core-security
bug*
Finally tracked the problem. Not s-s as I thought, so reopening.

We had thought in bug 934799 that lazy parsing only needs to turn off for JSD1 because JSD2's onNewScript hooks behaved nicer.

Usually, while parsing a direct eval script with an inner function, we will try to parse the inner function lazily. When the outer script is compiled (i.e. after CompileScript returns), we mark all inner functions as directlyInsideEval, so that we don't do GNAME => NAME optimizations when we get around to emitting bytecode for the inner functions. However, if Debugger is on and has a onNewScript hook which asks for its children scripts, as Nick's patch did, we end up calling the onNewScript hook *before* we get a chance to mark the outer script's inner objects.

I think we should just move that lazy script-inside-eval marking logic to inside CompileScript.
Group: javascript-core-security
Got the optimization backwards, it should read NAME => GNAME.
Assignee: nobody → shu
Status: NEW → ASSIGNED
Comment on attachment 8412478 [details] [diff] [review]
Mark inner lazy scripts inside eval scripts before firing Debugger's onNewScript hook

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

Oh wow, that is an annoying issue. Nice job on tracking it down.
Attachment #8412478 - Flags: review?(till) → review+
That is some epic debugging work. Nice job, Shu.
https://hg.mozilla.org/mozilla-central/rev/10e37b92e195
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Can we uplift this? People on Aurora and Beta are affected by this (ever since bug 978019 landed, IIUC).
Flags: needinfo?(shu)
And it's not just "oh they should be affected" it's "they are complaining on twitter because they actually are affected".
Comment on attachment 8412478 [details] [diff] [review]
Mark inner lazy scripts inside eval scripts before firing Debugger's onNewScript hook

Requesting uplift based on comment 28

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 934799 maybe?
User impact if declined: Incorrect behavior with devtools open
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): Should be none. not s-s
String or IDL/UUID changes made by this patch: None
Attachment #8412478 - Flags: approval-mozilla-aurora?
Flags: needinfo?(shu)
Attachment #8412478 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Are we just uplifting to Aurora, not Beta?
Word on the street is that there's going to be a respin. So you might want to request Fx30 approval ASAP if you want it to be considered.
Flags: needinfo?(shu)
Comment on attachment 8412478 [details] [diff] [review]
Mark inner lazy scripts inside eval scripts before firing Debugger's onNewScript hook

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 934799
User impact if declined: Inorrect behavior with devtools open
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): None. Not s-s.
String or IDL/UUID changes made by this patch: None.
Attachment #8412478 - Flags: approval-mozilla-beta?
Flags: needinfo?(shu)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #32)
> Word on the street is that there's going to be a respin. So you might want
> to request Fx30 approval ASAP if you want it to be considered.

Thanks for looking out. :)
This is not appropriate for a ride-along to the respin of final RC.  It could have landed earlier in the cycle but since this only presents when the Debugger is open (and the workaround would be to close the debugger) we can wait for this to ride up from FF31.  Will mark for release note as a known issue however.  I'm expecting that a lot of the affected users will be on Aurora/Beta and will actually have this fixed for them when we release next week and their versions are bumped.
Attachment #8412478 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Keywords: verifyme
Whiteboard: [firebug-p1] → [firebug-p1]
Works on Aurora now (and on trunk, last bad:1fe69cad9713, first good:10e37b92e195). Thanks a lot.
Verified fixed 31.0a2 (2014-06-09), 32.0a1 (2014-06-09), Win 7 x64
Status: RESOLVED → VERIFIED
Keywords: verifyme
Blocks: 1024806
No longer blocks: 1024806
You need to log in before you can comment on or make changes to this bug.