STR:

1. pause in the debugger
2. close devtools

ER: thread is resumed
AR: thread remains paused

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b3a69dff7e25f1c1107520c238dd282405afaf92&tochange=dd19d57a81d5690fccd5e45be09e3b8686d79e53

:jlast, if you think that's a regression, then could you try to find a regression range in using for example mozregression?

Yep, i added a regressed by

Alright, after investigating this a bit more, it looks like the issue originates from the destroy handler, which is a relief because that is not unsafeSyncrhronize territory.

Also, just to be safe, I double checked and the STR does not repro on June 5th, which was the date of the last change prior to the thread-client work.

Yulia, can you help unpack some of the onResume/doResume changes? It looks like there is now a subtle timing issue. Perhaps related to promises/async-await?

The regression range is: https://hg.mozilla.org/mozilla-central/rev/2494fafdeae721029ce499c9367f522c76aaa9c1

The cause is that, before the front and actor would be destroyed together. With the change to getFront, the destruction of the actor happened well after the front.

I added a fix but there is a problem -- we might actually test this somewhere, but the test passes. I am not sure what is different in our test environment and when testing manually, but the test included in the first patch here is exactly the steps that i used to reproduce this. I have included a test to demonstrate this. It might take some investigation into why the tests behave differently. We might have some cleanup for convenience.

Exceptions can be something else than Error.
An example could be Cu.Exception(), but native code can probably throw
unexpected things. It is also possible to throw any kind of object in JS...

pre and post nest functions are rather something specific to the thread actor
or the event loop classes. The only reason it is today on the target actors
is that the list of targetted windows can be different based on the target.
But we can figure out, from the thread actor's debuggees, the list of these
windows that are inspected by the thread actor.
I think that it is better that was as it better reflect what actual final
globals, the thread actor is interacting with. The previous code could possibly
introduce differences between globals for which we pauses the timeouts and events
versus the globals being inspected by the thread actor.

Use thread actor reference instead of target's actor URL and connection
to distinguish the currently paused event loop from the other ones which
may be on-pause from another thread actor on another connection/tab.
This happens when connecting more than one client against the same tab.

This appears ready to land. Are you going to request uplift?

Should we wait for the patch to bake in nightly for a few days before uplifting it right away?

BTW, I can't land the patches via lando because of this error message:

Landing Failed
There are landing blockers present which prevent landing.

Harald, it looks like I can't land because of the severity flag being set to blocker.
I don't know how to proceed, is this flag really relevant?

Weird. So the "landing blockers" weren't this bug, but something else.
First time I hit that. Does that mean, this bug is going to block the other pushes to autoland?
I didn't knew about this specialty of lando/autoland.

https://hg.mozilla.org/mozilla-central/rev/b123b923bf61
https://hg.mozilla.org/mozilla-central/rev/6d6ee45769d2
https://hg.mozilla.org/mozilla-central/rev/759b3ded0024

Hey Alexandre, since 69 is marked as affected, could you please nominate an uplift of this fix to beta69? Thanks!

Comment on attachment 9080318 [details]
Bug 1567210 - Ensure resuming page execution when the toolbox closes.

Beta/Release Uplift Approval Request

• User impact if declined: If the web developer hit a breakpoint and close the DevTools without manually resuming the page execution,
  the web page will still be blocked on the breakpoint, even if the Devtools are now closed.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: * open a page where you can set a breakpoint (for example: data:text/html,<script>window.onclick=()=>{debugger;};setInterval(()=>{document.body.innerHTML+="x<br/>"}, 1000)</script>
• open the debugger, set a breakpoint and trigger it (with the example, there is no need to set it, but just click in the page)
• close the devtools

Expected result: the page should be resume and work correctly:

• links should work
• in the example, the setInterval loop should be resume and "x" be printed
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This modifies some critical breakpoint code, but at the same time this overall modification is quite limited.
• String changes made/needed: no

Verified as fixed on Firefox Nightly 70.0a1 on Windows 10 x 64, Mac OS X 10.14 and on Ubuntu 18.04 x64.

Comment on attachment 9080318 [details]
Bug 1567210 - Ensure resuming page execution when the toolbox closes.

Fix verified on nightly by QA, has tests and fixes a major regression in devtools in 69, uplift approved for 69 beta 12, thanks!

Comment on attachment 9080318 [details]
Bug 1567210 - Ensure resuming page execution when the toolbox closes.

Alex, your uplift request is only for one of the 3 patches, can you confirm that this is intended? Thanks

(In reply to Pascal Chevrel:pascalc (pto Jul 12-30) from comment #22)

Comment on attachment 9080318 [details]
Bug 1567210 - Ensure resuming page execution when the toolbox closes.

Alex, your uplift request is only for one of the 3 patches, can you confirm that this is intended? Thanks

Yes. The two other patches are independant cleanup. So I'm only expecting to uplift this one patch.

Comment on attachment 9080318 [details]
Bug 1567210 - Ensure resuming page execution when the toolbox closes.

Fixes a bug which can cause a site to be blocked on a debugger breakpoint even if devtools were closed. Approved for 69.0b14.

https://hg.mozilla.org/releases/mozilla-beta/rev/377d42633864

Backed out from Beta for ESLint failures. Please attach a rebased Beta patch.
https://hg.mozilla.org/releases/mozilla-beta/rev/8ecc148a2fadc312c58f241222c0fcb024718827

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=261142350&repo=mozilla-beta&lineNumber=234

pre and post nest functions are rather something specific to the thread actor
or the event loop classes. The only reason it is today on the target actors
is that the list of targetted windows can be different based on the target.
But we can figure out, from the thread actor's debuggees, the list of these
windows that are inspected by the thread actor.
I think that it is better that was as it better reflect what actual final
globals, the thread actor is interacting with. The previous code could possibly
introduce differences between globals for which we pauses the timeouts and events
versus the globals being inspected by the thread actor.

Comment on attachment 9084995 [details]
Bug 1567210 - Ensure resuming page execution when the toolbox closes.

Here is a new beta specific patch that should be green on beta:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b56f3659c186c5dddc39950857a169a8f3b3f4ff

https://hg.mozilla.org/releases/mozilla-beta/rev/af6d4c6d2da1

Verified as fixed using Firefox Beta 69.0b14 (20190814024055) on Windows 10, Mac OS 10.14.6 and Ubuntu 18.04.
Build from: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision