1299602 - Debugger fails to emit tabNavigated event when paused

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Jason Laster [:jlast]]

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/

Comment 1

[J. Ryan Stinnett [:jryans]]

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

Comment 2

[Alexandre Poirot [:ochameau]]

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?

Comment 3

[Alexandre Poirot [:ochameau]]

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.

Comment 4

[Alexandre Poirot [:ochameau]]

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?

Comment 5

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3070c465d41b

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1299602 - Do not suspend navigation when navigating with a breakpoint.

Review commit: https://reviewboard.mozilla.org/r/75994/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/75994/

Comment 7

[Alexandre Poirot [:ochameau]]

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

Comment 8

[Alexandre Poirot [:ochameau]]

For what it is worth, try looks green on linux.

Comment 9

[James Long (:jlongster)]

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

[James Long (:jlongster)]

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

Comment 11

[Alexandre Poirot [:ochameau]]

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

Comment 12

[Alexandre Poirot [:ochameau]]

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42ada9dc0ab3
Do not suspend navigation when navigating with a breakpoint. r=jlongster

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42ada9dc0ab3

Comment 15

[James Long (:jlongster)]

Thanks so much Alex!