Closed Bug 1299602 Opened 3 years ago Closed 3 years ago

Debugger fails to emit tabNavigated event when paused

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: jlast, Unassigned)

References

Details

Attachments

(1 file)

When the debugger is paused, refreshing the debuggee does not emit a tabNavigated event.

It looks like it's due to an error in request.suspend() that prevents the event from being emitted.

https://www.irccloud.com/pastebin/CaJRoQ5q/
Priority: -- → P2
Flags: needinfo?(poirot.alex)
Blocks: 1294139
My STR to produce the linked stack was:

1. Open Debugger
2. Set a breakpoint in your favorite script and trigger it to hit
3. Reload the page (Cmd-R) while paused at the breakpoint
Whaaa you are hitting a old piece of code that predates me:
  https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webbrowser.js#1951

Looks like we are trying to:
 1) pause the request  <=== but we throw here!
 2) somewhat synchronously resume js execution
 3) on event loop wake up, resume the request

May be it was still working in non-e10s but it now throws in e10s?
Flags: needinfo?(poirot.alex)
I can surely make a patch that remove this complexity and stop messing up with the request,
but it would be better to know why we had to do that in the past and this is very related to the debugger guts, which I don't know that much about.
Looking at blame. It began in bug 751949, because of an assertion and things being broken.

It is much different now in e10s as the request ends up happening in the parent.
May be we could let the request go and "just" resume the debugger?
Depends on: 751949
More like a feedback request than review as I expect that to break things on non-e10s.
And I don't really know the consequences of not suspending the request...
For what it is worth, try looks green on linux.
(In reply to Alexandre Poirot [:ochameau] from comment #3)
> I can surely make a patch that remove this complexity and stop messing up
> with the request,
> but it would be better to know why we had to do that in the past and this is
> very related to the debugger guts, which I don't know that much about.

I agree, I have no idea why it was trying to suspend it before and unfortunately there are no comments. I guess the only path forward is to stop doing it, as per your patch, and we'll learn the reason why it was doing it before. It may have been a hack before that we don't need to do anymore.
Comment on attachment 8787130 [details]
Bug 1299602 - Do not suspend navigation when navigating with a breakpoint.

https://reviewboard.mozilla.org/r/75994/#review74026

What kinds of thing to do you expect to break in non-e10s? I'm assuming that you ran this locally and it does fix the problem? You could turn of e10s and at least make sure it's not totally broken. If try is green, that seems good enough...
Attachment #8787130 - Flags: review?(jlong) → review+
(In reply to James Long (:jlongster) from comment #10)
> Comment on attachment 8787130 [details]
> Bug 1299602 - Do not suspend navigation when navigating with a breakpoint.
> 
> https://reviewboard.mozilla.org/r/75994/#review74026
> 
> What kinds of thing to do you expect to break in non-e10s? 

I was expecting to resurect bug 751949. But that's only an assertion, not an exception.
I'll try to build a debug build to see if it revives the assertion.

Testing with this doc:
data:text/html,<script>onclick=function () {%0Aconsole.log("plop");%0A}</script>

Setting a breakpoint at line two, on the console.log(),
reloading after hitting the breakpoint doesn't seem to introduce any breakage nor exception no matter if e10s is turned on or off.
I do not see any assertion on debug build either.
Let's proceed, but keep this patch in mind if you see anything unexpected happening on reload.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42ada9dc0ab3
Do not suspend navigation when navigating with a breakpoint. r=jlongster
https://hg.mozilla.org/mozilla-central/rev/42ada9dc0ab3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Thanks so much Alex!
Depends on: 1300036
Duplicate of this bug: 1254613
Duplicate of this bug: 1189111
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.