Closed Bug 1269457 Opened 8 years ago Closed 8 years ago

Setting a breakpoint anywhere in the Browser Toolbox debugger causes the browser to jank hard

Categories

(DevTools :: Debugger, defect, P2)

48 Branch
defect

Tracking

(firefox47 unaffected, firefox48- fixed, firefox49+ fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox47 --- unaffected
firefox48 - fixed
firefox49 + fixed

People

(Reporter: mconley, Assigned: jryans)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:

1) Open up the Browser Toolbox
2) Switch to the debugger
3) Set a breakpoint anywhere. Doesn't need to be code you'll hit.
4) Go back to the browser window and start switching tabs or trying to do things.

ER:

Snappy front-end, as per usual.

AR:

The main process seems to be really bogged down.

Here's a profile:

https://cleopatra.io/#report=8d8c11b098b5942f4b0757591cbcdbe0cf9c9309&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A18225859,%22end%22%3A18227113%7D%5D&selection=0,1,2,3,4,3,5,3,6,3,7,3,8,3,9,3,10,3,11,3,12,3,13,3,14,15,3,16,17,141,142,3,146,3,148,18,3,19,20,21,22,23

I'm seeing lots of long calls to ScriptStore.js's getScriptsBySourceActorAndLine.

I'm afraid this is really hampering my ability to debug front-endy things.
Bisected this down to bug 1225160.

jryans! Any idea what's going on here?
Blocks: 1225160
Flags: needinfo?(jryans)
Keywords: regression
:ejpbruel should have a better idea than me, let's try him.
Flags: needinfo?(jryans) → needinfo?(ejpbruel)
I did a local backout of both of the patches from bug 1225160, and it seems pretty definitive that they're introducing the regression I'm hitting here.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> I did a local backout of both of the patches from bug 1225160, and it seems
> pretty definitive that they're introducing the regression I'm hitting here.

Hi Mike,

The patches in bug 1225160 disable source maps by default for the chrome debugger, and carefully avoids the use of nested event loops when source maps are disabled.

The reason for this is that source maps introduce asynchronous code to the debugger in places where it expects synchronous behavior. We used nested event loops to maintain synchronous behavior in the presence of asynchronous code, but these nested event loops can have very nasty side effects. In particular, it can cause a JSM to be loaded while it is already in the process of being loaded. The browser is not prepared for this, so when that happens it crashes completely. Avoiding this turns out to be extremely complicated, and this patch was the best short-term solution we could come up with.

The patch significantly changes the behavior of the debugger: it no longer uses asynchronous code when source maps are disabled. It also uses different code paths when source maps are disabled. It's not unthinkable that this would have unforeseen side-effects.

I'd like to see this bug fixed asap, but our main focus for this quarter is the devtools.html push. I'm assigning P2 to this so we can pick it up if we have any free cycles.
Flags: needinfo?(ejpbruel)
Priority: -- → P2
I just filed bug 1273907 before realizing this had already been filed. I'm seeing the same thing and, in my particular use case, it's making debugging the tab toolbox with the browser toolbox really hard.
Whenever I set a breakpoint in the browser toolbox and then open the tab toolbox the browser freezes for ~30 seconds (even sometimes shows the slow script dialog).
It's not at all clear to me why the changes in bug 1225160 should have this effect. The server should be doing strictly *less* work after those patches, not more, right?
Flags: needinfo?(ejpbruel)
If bug 1225160 is the cause of the regression then it affects 48 too. If we get a fix, we should verify and uplift it.  Eddy, if you can't take this on, can you find another owner for the bug so we avoid shipping this regression to release?
Liz, it may not be obvious, but this only occurs when people set breakpoints in Firefox's chrome code itself, not when web developers are using the devtools on content. So it affects add-on developers and Firefox developers, but not web developers. 

I don't know how that affects urgency; maybe we want it fixed in 48 anyway. But I wanted to make sure that was clear.
Flags: needinfo?(lhenry)
Thanks, release management may not need to be tracking this at all then. I just noticed it while triaging through regressions in nightly.
Flags: needinfo?(lhenry)
(In reply to Jim Blandy :jimb from comment #7)
> It's not at all clear to me why the changes in bug 1225160 should have this
> effect. The server should be doing strictly *less* work after those patches,
> not more, right?

That's my assessment too.

When source maps are enabled, we have to load a source map, use a nested event loop to wait for the source map load to complete, translate the original location of each breakpoint into a generated location, and then set the breakpoint on those generated locations.

When source maps are disabled, we don't have to load source maps, we don't have to use nested event loops to wait for the source map loading to complete, and we just set the breakpoints on the original location directly.

All of that should translate to strictly less work. The fact that it's not suggests that we are doing something very inefficient in one of the code paths that are taken when source maps are disabled. It's not immediately obvious to me what that something could be.
Flags: needinfo?(ejpbruel)
The special code path carved out in bug 1225160 dropped the `actor.isPending`
check which causes many, many attempts to set a breakpoint on every new source
notification, leading to a very slow debugging experience.

Review commit: https://reviewboard.mozilla.org/r/56656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56656/
Attachment #8758434 - Flags: review?(ejpbruel)
Comment on attachment 8758434 [details]
MozReview Request: Bug 1269457 - Only set breakpoints for pending bp actors. r=ejpbruel

https://reviewboard.mozilla.org/r/56656/#review53624

Thanks for catching this, Ryan!
Attachment #8758434 - Flags: review?(ejpbruel) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/369f9791b11e
Only set breakpoints for pending bp actors. r=ejpbruel
https://hg.mozilla.org/mozilla-central/rev/369f9791b11e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8758434 [details]
MozReview Request: Bug 1269457 - Only set breakpoints for pending bp actors. r=ejpbruel

Approval Request Comment
[Feature/regressing bug #]: Bug 1225160
[User impact if declined]: Debugging with source maps disabled will be very, very slow without this patch.
[Describe test coverage new/current, TreeHerder]: On m-c, manual tested.
[Risks and why]: Low, specific to debugger, small change.
[String/UUID change made/needed]: None
Attachment #8758434 - Flags: approval-mozilla-aurora?
Assignee: nobody → jryans
Comment on attachment 8758434 [details]
MozReview Request: Bug 1269457 - Only set breakpoints for pending bp actors. r=ejpbruel

Recent regression, taking it
Attachment #8758434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Version: unspecified → 48 Branch
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.