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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[andershol]

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)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This only seems to happen when the debugger is open; if I try to repro w/ console or inspector the map loads.

Comment 2

[andershol]

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

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yup, bug 978019 introduced this. No idea why though... this seems insane to me. Thanks for bisecting!

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 8

[Brian Grinstead [:bgrins]]

(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?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Testing right now whether it is the |_addScript| call or the |getChildScripts| call...

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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...

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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)?

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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.

Comment 14

[Shu-yu Guo [:shu]]

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.

Comment 15

[Shu-yu Guo [:shu]]

Does anything in devtools intercept DOM calls and might mess with the dynamically created DOM?

Comment 16

[Shu-yu Guo [:shu]]

Attachment: 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.

Comment 17

[Shu-yu Guo [:shu]]

$'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?

Comment 18

[Shu-yu Guo [:shu]]

Closing but, because it seems like we're leaking a chrome global.

Comment 19

[Shu-yu Guo [:shu]]

bug*

Comment 20

[Shu-yu Guo [:shu]]

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.

Comment 21

[Shu-yu Guo [:shu]]

Got the optimization backwards, it should read NAME => GNAME.

Comment 22

[Shu-yu Guo [:shu]]

Attachment: Mark inner lazy scripts inside eval scripts before firing Debugger's onNewScript hook

See comment 20 for explanation.

Comment 23

[Till Schneidereit [:till]]

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.

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

That is some epic debugging work. Nice job, Shu.

Comment 25

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10e37b92e195

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10e37b92e195

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Can we uplift this? People on Aurora and Beta are affected by this (ever since bug 978019 landed, IIUC).

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

And it's not just "oh they should be affected" it's "they are complaining on twitter because they actually are affected".

Comment 29

[Shu-yu Guo [:shu]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e812918523be

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Are we just uplifting to Aurora, not Beta?

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 33

[Shu-yu Guo [: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.

Comment 34

[Shu-yu Guo [: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. :)

Comment 35

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 36

[andershol]

Works on Aurora now (and on trunk, last bad:1fe69cad9713, first good:10e37b92e195). Thanks a lot.

Comment 37

[Paul Silaghi, QA [:pauly]]

Verified fixed 31.0a2 (2014-06-09), 32.0a1 (2014-06-09), Win 7 x64

Comment 38

[Kohei Yoshino]

I have seen some reports on this:
https://docs.google.com/spreadsheets/d/1048AVpvbP3O2atsV3i--xLpHCr4PR7TGXyFtgMrvt6Q/edit#gid=89642089

Added to the site compat doc:
https://developer.mozilla.org/en-US/Firefox/Releases/30/Site_Compatibility#JavaScript
https://twitter.com/FxSiteCompat/status/477225551678017537