1163854 - Debugger can cause layout to be recursively entered

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Shu-yu Guo [:shu]]

mrbkap found this bug where a Debugger attached to chrome globals can cause layout to be re-entered, which will crash.

Blake's STR to crash:

1) Open Browser Toolbox and go to the Debugger tab.
2) Reload something

It would then crash in some awesome bar-related chrome code.

Artist's rendition of the re-entered layout stack is as follows:

  Layout                 (C++)
  Event Loop             (C++)
  synchronize            (JS)
  Debugger::onNewScript  (JS)
  XBL cloning getters    (C++)
  Layout                 (C++)

That is, while processing XBL under layout, a cloned script triggers Debugger.onNewScript, which re-enters JS. The debugger server then spins the event loop using |synchronize| (!) in an attempt to fetch source maps. This pumps the event loop, which then can cause us to re-enter layout, where we crash.

I'm not sure what the right bandaid here is.

Re-entering JS under layout is a footgun and should be disallowed regardless of what bandaid we choose for this current bug.

Talking briefly with Jim on IRC, we proposed 2 things:

1) Have some RAII struct that guarantees no JS-reentry.
2) Enqueue Debugger events in the scope of this RAII struct and fire them on its destruction.

Comment 1

[Jim Blandy :jimb]

(In reply to Shu-yu Guo [:shu] from comment #0)
> Artist's rendition of the re-entered layout stack is as follows:
http://upload.wikimedia.org/wikipedia/commons/3/3f/Schongauer,_Martin_-_St_Antonius_-_hi_res.jpg

Comment 2

[Jim Blandy :jimb]

Note bug 1163798, which would move sourcemap parsing into a web worker. That would remove the need for the 'synchronize' calls, which are from the devil.

Comment 3

[Jim Blandy :jimb]

Actually, wait, it wouldn't remove the synchronize calls at all. :( Communication with workers is never synchronous.

Comment 4

[James Long (:jlongster)]

cc'ing Eddy, we've talked about this specific part of the debugger a lot.

We've got to get rid of synchronize. I don't really care about the sacrifices we have to make.

I've been unsure about using synchronize inside of `_addScript` to block on sourcemaps in the `onNewScript` hook since we introduced this. I think this is a pretty heavy strike against it.

We should not sacrifice too much to the sourcemaps gods. They should be consulted, not worshiped. Their laws are impossible; they live in a fake dimension barely resembling our reality.

Basically: we should be ok with sacrificing correctness. It's just impossible to wait on sourcemaps to set breakpoints when a new script is created. Let's not worry so much about sourcemaps.

Developers who use sourcemaps have a mental model with how they work; it's almost expected that this won't work because they know sourcemaps are fetched & parsed asynchronously, while breakpoints need to be immediately set. Matching this mental model is actually more helpful because sourcemaps are a pain and ass and everyone knows it, as long as things are consistent when they should be in this mental model (knowing it's async, etc), we're fine.

Everyone loves Chrome's sourcemap support so we should just do what they do. They don't wait on sourcemaps, they immediately set the breakpoint. Yes, this causes some problems if the underlying sources changed, as I show here in the screenshot. Who cares.

Comment 5

[James Long (:jlongster)]

Attachment: Screen Shot 2015-05-11 at 8.16.06 PM.png

this happens when I set breakpoint, change a *different* JS file which rebuilds the entire generated JS file and kicks this location off, and reload the page. It just sets the old location. It's straight-forward and understandable, even if technically it's not correct. Devs expect this in the mental model they have about sourcemaps.

Comment 6

[James Long (:jlongster)]

Also, for all the cases where we are in a debugger hook and we need access to the sourcemaps, we should just asynchronously wait for it. Pause everything, tell the client, and then wait for the sourcemap to come in. That's what Chrome does, and it has a message saying that it's waiting for the sourcemap. This is better than using `synchronize` to block until the entire sourcemap is available, and would make it feel snappier too.

Comment 7

[James Long (:jlongster)]

I filed bug 1164911 to suggest making all of our sourcemap interactions async.

Comment 8

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

Was just talking with Jim and Shu on IRC about this bug and we came up with a relatively confined hackaround to avoid crashing.

We make createSourceMappedActors return null/empty list/whatever when we aren't source mapping the given script/source, and return the promise of the list of sources (ie, current behavior) when we are actually source mapping. Then the caller can handle these cases and only spin the event loop via synchronize in the latter case. But notably the XBL scripts aren't ever source mapped and so we avoid this crasher.

Comment 9

[Jason Laster [:jlast]]

Closing as source maps are handled in the client now.