Closed Bug 896139 Opened 11 years ago Closed 9 years ago

Breakpoints not triggering when reloading script

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: jimb, Assigned: jlong)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

I have a web page where pressing a button runs the following code:

var s = document.createElement('script');
s.setAttribute('src', 'alert.js');
document.body.appendChild(s);

and alert.js contains the following code:

alert("alert.js's main script is running!");

When I open the debugger and press the button, the alert dialog pops up, and alert.js appears in my list of sources. But if I set a breakpoint on line 1 of alert.js and press the button again, the breakpoint never hits, and the alert dialog appears immediately.

Technically, it's a different <script> element, but I think web developers expect that if they've set a breakpoint on line 1 of alert.js, then they expect a pause when they hit line 1 of alert.js. N'est-ce pas?

I think this is because we don't properly apply existing breakpoints when new scripts appear. ThreadActor.prototype._addScript scans the table of existing breakpoint locations, but if the BreakpointActor has already been inserted in any existing scripts, then we don't call _setBreakpoint on it - thus, the second <script> element's instance of alert.js gets passed over, because the breakpoint we set on the first <script> already has a Debugger.Script in its instance list.

However, simply calling _setBreakpoint doesn't solve the problem either: inserting a breakpoint twice actually produces *two* entries for the given breakpoint object at that bytecode, and you'll get multiple breakpoint hits for the same breakpoint when control reaches that location --- another one each time a new script with the right URL gets loaded.

The right fix is probably to refactor _setBreakpoint into one function that applies the breakpoint to all extant relevant D.Script instances --- used when first setting a breakpoint --- and a simpler function that just sets a breakpoint in a specific Debugger.Script instance. Then _addScript would simply walk the tree of scripts rooted at the new script it had just been presented with, and set the breakpoint in those fresh scripts alone --- no need for a whole-compartment scan.
Priority: -- → P2
Summary: JS debugger: breakpoints in reloaded scripts don't trigger → Breakpoints not hitting inside reloaded scripts
Summary: Breakpoints not hitting inside reloaded scripts → Breakpoints not triggering when reloading script
Indeed. Firebug gets this right, so when I saw the announcement about Moz Dev Edition, and started trying out its debugger, I ran into this deficiency.

In addition to the case described, this problem occurs when <script> tags are used inside <body>, as recommended when you want a splash screen by this page: http://limitless-lowlands-6996.herokuapp.com/http://www.ng-newsletter.com/advent2013/%3F_escaped_fragment_=/day/21?_escaped_fragment_=/day/21#!/day/21

And that recommendation is highly useful for single-page applications, where there is a fair amount of javascript to load, and loading it in <head> would mean that by the time the splash screen appears, it is too late to be useful.
(In reply to Jim Blandy :jimb from comment #0)
> I have a web page where pressing a button runs the following code:
> 
> var s = document.createElement('script');
> s.setAttribute('src', 'alert.js');
> document.body.appendChild(s);
> 
> and alert.js contains the following code:
> 
> alert("alert.js's main script is running!");
> 
> When I open the debugger and press the button, the alert dialog pops up, and
> alert.js appears in my list of sources. But if I set a breakpoint on line 1
> of alert.js and press the button again, the breakpoint never hits, and the
> alert dialog appears immediately.
> 
> Technically, it's a different <script> element, but I think web developers
> expect that if they've set a breakpoint on line 1 of alert.js, then they
> expect a pause when they hit line 1 of alert.js. N'est-ce pas?
> 
> I think this is because we don't properly apply existing breakpoints when
> new scripts appear. ThreadActor.prototype._addScript scans the table of
> existing breakpoint locations, but if the BreakpointActor has already been
> inserted in any existing scripts, then we don't call _setBreakpoint on it -
> thus, the second <script> element's instance of alert.js gets passed over,
> because the breakpoint we set on the first <script> already has a
> Debugger.Script in its instance list.
> 
> However, simply calling _setBreakpoint doesn't solve the problem either:
> inserting a breakpoint twice actually produces *two* entries for the given
> breakpoint object at that bytecode, and you'll get multiple breakpoint hits
> for the same breakpoint when control reaches that location --- another one
> each time a new script with the right URL gets loaded.
> 
> The right fix is probably to refactor _setBreakpoint into one function that
> applies the breakpoint to all extant relevant D.Script instances --- used
> when first setting a breakpoint --- and a simpler function that just sets a
> breakpoint in a specific Debugger.Script instance. Then _addScript would
> simply walk the tree of scripts rooted at the new script it had just been
> presented with, and set the breakpoint in those fresh scripts alone --- no
> need for a whole-compartment scan.

My refactor of the breakpoint code seemed to have fixed this.

If the BreakpointActor has already been set as a breakpoint handler on at least one script, it is no longer considered pending, so we wont call _setBreakpoint (which can perform breakpoint sliding) on it.

However, its still possible for previously gcd scripts to be reintroduced for which the BreakpointActor needs to be set as a breakpoint handler, even though its no longer pending. To handle this case, we call setBreakpointAtGeneratedLocation. This function finds all scripts that correspond to the BreakpointActor, and ensures it is set as a breakpoint handler for each of them (ignoring scripts for which the actor has already been set as a handler).

This wasn't intentional, but it looks like the latter case also handles the case where a new script is introduced with the same url.

This works for me on the latest nightly. Jim, can you confirm that it works for you too, so I can mark this bug as resolved?
Flags: needinfo?(jimb)
Assignee: nobody → ejpbruel
Is it too much to ask for landing a regression test for the accidentally-fixed bug?
Do we know when the fix landed? Not covering this case is a pretty bad omission from our test suite; it's hardly unusual. I don't think this bug should be closed until we have a regression test that's been shown to fail in prior versions.
Flags: needinfo?(jimb)
But yes, the bug is fixed in nightly.
(In reply to Jim Blandy :jimb from comment #4)
> Do we know when the fix landed? Not covering this case is a pretty bad
> omission from our test suite; it's hardly unusual.

We do have https://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_breakpoints-reload.js?from=browser_dbg_breakpoints-reload.js

I think part of the issue is that we attempt to show all html sources in a single unified source on the frontend, when really they are many different sources. The problems arise when you dynamically append script elements with inline text contents, and we try to synthesize it within the unified html source and it is just :(

We should just show each source as its own source instead of trying to unify the html sources...
(In reply to On vacation until May 17th from comment #6)
> (In reply to Jim Blandy :jimb from comment #4)
> > Do we know when the fix landed? Not covering this case is a pretty bad
> > omission from our test suite; it's hardly unusual.
> 
> We do have
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/
> test/browser_dbg_breakpoints-reload.js?from=browser_dbg_breakpoints-reload.js
> 
> I think part of the issue is that we attempt to show all html sources in a
> single unified source on the frontend, when really they are many different
> sources. The problems arise when you dynamically append script elements with
> inline text contents, and we try to synthesize it within the unified html
> source and it is just :(
> 
> We should just show each source as its own source instead of trying to unify
> the html sources...

I've been annoyed by that hack ever since I first discovered it, so I'm volunteering to remove it, provided James and Jim also agree that it should be removed.
Flags: needinfo?(jimb)
James, do you think we should stop trying to unify the HTML sources or not?
Flags: needinfo?(jlong)
Not until we refactor the source pane which we really need to do ASAP. The UX would be way too confusing otherwise. Our sources pane sucks and that would just make it worse.
Flags: needinfo?(jlong)
I'm not too current on the debugger's UX, so I would defer to James and Eddy, but I don't object.
Flags: needinfo?(jimb)
Attached patch Regression test (obsolete) — Splinter Review
Here is the regression test you asked for.

Because it was part of a rather large refactor, I don't know exact commit that made this bug go away. I could do a bisect in order to find it, but it doesn't seem worth the trouble to me.

At least this test will make sure that the behavior described in this bug now works as intended.
Attachment #8601952 - Flags: review?(jimb)
Comment on attachment 8601952 [details] [diff] [review]
Regression test

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

Very yield. Such legible. Wow.
Attachment #8601952 - Flags: review?(jimb) → review+
(In reply to Wes Kocher (:KWierso) from comment #14)
> The test this patch adds is timing out, so I backed it out in
> https://hg.mozilla.org/integration/fx-team/rev/f8ad92bbeea6
> 
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=2973302&repo=fx-team

I added a copyright comment to one of the test files before landing. What could possibly go wrong, right? Surely I can avoid a try push this time.

*TREE CATCHES FIRE*

Aaaand that changed the line number of one of the breakpoints the test is setting.

Made a try push before landing again. I learned my lesson. (At least for the next few days)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=510fd5ed8cf3
Flags: needinfo?(ejpbruel)
Attached patch 896139.patch (obsolete) — Splinter Review
The last test was failing on e10s and exposed a problem with dbg-client's setBreakpoint function: it doesn't ensure that the debugger was in the same state that it was called in. It resumes the debugger but doesn't wait for that protocol request to complete before calling the callback. I changed that and it should work.

It's a little bit of a scary change but I don't see how this is isn't better. We'll see if any other tests fail.
Attachment #8601952 - Attachment is obsolete: true
Attachment #8603449 - Flags: review?(ejpbruel)
I'll make sure to push this through
Assignee: ejpbruel → jlong
Comment on attachment 8603449 [details] [diff] [review]
896139.patch

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

Looks good to me, just make sure this doesn't break any other tests. But you're already doing that.

Thanks for helping me figure this out!
Attachment #8603449 - Flags: review?(ejpbruel) → review+
Arggggh of course a bunch of our other tests depend on this brittle order of execution. The main problem stems from the confusion of "attached" and "resumed" states and that we use `ensureThreadClientState` everywhere with "resumed" which is bogus because what they really mean is `waitForThreadEvents(panel, "resumed")` because "resumed" is not an actual state.

It's so confusing and I've filed bug 1156531 about this discrepancy.

For now, let's turn this test off for e10s and I'll make a note to turn it back on when I fix bug 1156531.
Actually, now that I understand what's going on I can just be more explicit in the test and I should be able to get it working under e10s.
Attached patch 896139.patchSplinter Review
new non-race-conditiony-under-e10s patch
Attachment #8603449 - Attachment is obsolete: true
Yay it's fixed! Comparing the above 2 try runs makes it obvious. The failures in the latest try run look completely unrelated.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/24e6fdc5e7c1
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/24e6fdc5e7c1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.