1571637 - Closing devtools should resume the event loop we stop at

Status: NEW

(DevTools :: Debugger, defect, P3)

Description

[Alexandre Poirot [:ochameau]]

While verifying bug 1567210 I saw a couple of issues around breakpoints.

While the page is now correctly resumed in term of timeouts/intervals and page events,
the actual event loop we stopped at is not resumed when closing the DevTools.

STR:

• open: data:text/html,<script>window.onclick=()=>{%0Adocument.body.innerHTML+="x";%0A};</script>
• open the debugger, break at document.body.innerHTML+="x"; line
• click on the page in order to trigger the breakpoint
• the page should be paused and "x" is not printend on the page
• now close the DevTools

Expected result: the breakpoint is released and "x" is displayed.

Current result on nightly: "x" is not displayed, which seems to indicated that the breakpoint isn't correctly released

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Action loadSourceText had an exception

Thanks for the report Alex, I can reproduce that on my machine.

I am also seeing an error message in the Browser Console. Attached in case it's relevant.

Honza

Comment 3

[Logan Smyth [:loganfsmyth]]

@ochameau Curious for your thoughts here. This is caused by https://searchfox.org/mozilla-central/rev/171109434c6f2fe086af3b2322839b346a112a99/devtools/server/actors/thread.js#846 returning null and thus stopping execution.

Is this code still something that actually does anything useful? I ask because we only appear to set _parentClosed on exit (https://searchfox.org/mozilla-central/rev/171109434c6f2fe086af3b2322839b346a112a99/devtools/server/actors/targets/browsing-context.js#550), not on page navigation, meaning that most of the time it doesn't actually do the job of stopping "Executing JS after the content window is gone" anyway, so if it were a critical piece of functionality it seems like it would already be a problem? We also already use pauseAndRespond in a few other places where the return value is ignored, like in XHR, DOM, and Event breakpoints, because they pause inside deep C++ code, not inside a Debugger.cpp hook and so don't have a useful way to cancel execution anyway.

Would it make sense to remove this "feature" of returning null, and then see if we actually run into cases where the window responds badly at this point in time?

Comment 4

[Alexandre Poirot [:ochameau]]

Yes this _parentClosed attribute is probably a poorly maintained relic of the ancient times.
First, it should not be maintained by WindowGlobalTargets.
Then, you are probably right, the current usage of it from the ThreadActor is probably irrelevant.
While there might be an argument to keep the event loop stuck when the related document is destroyed,
this shouldn't apply to devtools being closed.

It would be nice to start with writing a test to cover this.