Remote Protocol tests currently have the new modal UI disabled. This issue is for updating the browser_javascriptDialog*.js tests to work with the new one.

The problem here is not the test itself but the actual implementation in the Remote Agent:
https://searchfox.org/mozilla-central/source/remote/domains/parent/page/DialogHandler.jsm

Note that when starting to add the feature we will have to remove the added preference here:
https://searchfox.org/mozilla-central/source/remote/RecommendedPreferences.jsm

Considering that for now the new modal dialog is only enabled on some channels, I guess Remote Agent & Marionette need to transparently support both implementations.

Backed out changeset 0fccc6a4922d (bug 1686743) for remote failures on browser_javascriptDialog_*.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&fromchange=8da69dee2955a43bb1a144fb136e19e74b6830db&searchStr=remote&tochange=3b1ab9b78237c0f090e9a13154280ee1c17283ef&selectedTaskRun=K4c7AX9SSROPTPcjmJwC7g.0

Backout link: https://hg.mozilla.org/integration/autoland/rev/4c10be0d54a97192b8fcb131878e5518511806fb

Failure log: https://treeherder.mozilla.org/logviewer?job_id=335178454&repo=autoland&lineNumber=25969

...
...
...
[task 2021-04-01T17:11:12.192Z] 17:11:12     INFO - GECKO(1537) | 1617297072121	RemoteAgent	TRACE	<-(connection {372ffbe5-c279-4dca-ad8e-f9522523f10e}) {"method":"Target.targetDestroyed","params":{"targetId":"0c22cb33-0dc9-49b7-9fcb-c2608a85de32"}}
[task 2021-04-01T17:11:12.193Z] 17:11:12     INFO - GECKO(1537) | 1617297072121	RemoteAgent	TRACE	<-(connection {3d9ca30a-9e59-4952-865d-42ce1377503d}) {"method":"Target.targetDestroyed","params":{"targetId":"0c22cb33-0dc9-49b7-9fcb-c2608a85de32"}}
[task 2021-04-01T17:11:12.193Z] 17:11:12     INFO - GECKO(1537) | 1617297072121	RemoteAgent	TRACE	<-(connection {190e7ab2-b81d-41e9-8cc7-b1ce8bb0fca6}) {"method":"Target.targetDestroyed","params":{"targetId":"0c22cb33-0dc9-49b7-9fcb-c2608a85de32"}}
[task 2021-04-01T17:11:12.195Z] 17:11:12     INFO - GECKO(1537) | 1617297072122	RemoteAgent	TRACE	<-(connection {7275eb23-4bd3-41c8-80ad-fa863d11712f}) {"method":"Target.targetDestroyed","params":{"targetId":"0c22cb33-0dc9-49b7-9fcb-c2608a85de32"}}
[task 2021-04-01T17:11:12.195Z] 17:11:12     INFO - GECKO(1537) | [Parent 1537, Main Thread] WARNING: NS_ENSURE_TRUE(mReady) failed: file /builds/worker/checkouts/gecko/xpfe/appshell/nsWindowMediator.cpp:161
[task 2021-04-01T17:11:12.196Z] 17:11:12     INFO - GECKO(1537) | 1617297072122	RemoteAgent	ERROR	unable to stop listener: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIWindowMediator.getEnumerator]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://remote/content/cdp/observers/TargetObserver.jsm :: stop :: line 64"  data: no] Stack trace: stop()@TargetObserver.jsm:64
[task 2021-04-01T17:11:12.196Z] 17:11:12     INFO - GECKO(1537) | unwatchForTabs()@TargetList.jsm:70
[task 2021-04-01T17:11:12.196Z] 17:11:12     INFO - GECKO(1537) | unwatchForTargets()@TargetList.jsm:37
[task 2021-04-01T17:11:12.196Z] 17:11:12     INFO - GECKO(1537) | destructor()@TargetList.jsm:109
[task 2021-04-01T17:11:12.196Z] 17:11:12     INFO - GECKO(1537) | close()@RemoteAgent.jsm:136
[task 2021-04-01T17:11:13.544Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 25 (7f7f93c4d040) [pid = 1537] [serial = 255] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.545Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 24 (7f7fa0b8c900) [pid = 1537] [serial = 228] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.546Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 23 (7f7fa1081740) [pid = 1537] [serial = 238] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.546Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 22 (7f7f93c4d580) [pid = 1537] [serial = 262] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.547Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 21 (7f7f93cdd040) [pid = 1537] [serial = 272] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.547Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 20 (7f7f936373c0) [pid = 1537] [serial = 293] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.548Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 19 (7f7f93637ac0) [pid = 1537] [serial = 303] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.548Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 18 (7f7f93613580) [pid = 1537] [serial = 245] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.549Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 17 (7f7fb19ecc00) [pid = 1537] [serial = 256] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.549Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 16 (7f7fa9a61800) [pid = 1537] [serial = 233] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.549Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 15 (7f7f9b731400) [pid = 1537] [serial = 239] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.553Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 14 (7f7fc6f5d400) [pid = 1537] [serial = 267] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.553Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 13 (7f7f9a10e400) [pid = 1537] [serial = 273] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.554Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 12 (7f7f938cac00) [pid = 1537] [serial = 298] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.554Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 11 (7f7f9a10d400) [pid = 1537] [serial = 304] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.554Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 10 (7f7fa9a81c00) [pid = 1537] [serial = 250] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.555Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 9 (7f7fb1963400) [pid = 1537] [serial = 2] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.556Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 8 (7f7fb1908900) [pid = 1537] [serial = 1] [outer = 0] [url = chrome://browser/content/browser.xhtml]
[task 2021-04-01T17:11:13.559Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 7 (7f7fb1908040) [pid = 1537] [serial = 14] [outer = 0] [url = chrome://mochikit/content/browser-harness.xhtml]
[task 2021-04-01T17:11:13.560Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 6 (7f7f9ecd3400) [pid = 1537] [serial = 15] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.560Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 5 (7f7fbf659040) [pid = 1537] [serial = 3] [outer = 0] [url = chrome://extensions/content/dummy.xhtml]
[task 2021-04-01T17:11:13.561Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 4 (7f7fb19e5400) [pid = 1537] [serial = 4] [outer = 0] [url = chrome://extensions/content/dummy.xhtml]
[task 2021-04-01T17:11:13.562Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 3 (7f7fddf6d200) [pid = 1537] [serial = 7] [outer = 0] [url = resource://gre-resources/hiddenWindow.html]
[task 2021-04-01T17:11:13.562Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 2 (7f7fbf6593c0) [pid = 1537] [serial = 5] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.563Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 1 (7f7f93463800) [pid = 1537] [serial = 436] [outer = 0] [url = about:blank]
[task 2021-04-01T17:11:13.564Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 0 (7f7fa4b4c400) [pid = 1537] [serial = 9] [outer = 0] [url = resource://gre-resources/hiddenWindow.html]
[task 2021-04-01T17:11:13.572Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp:3365
[task 2021-04-01T17:11:13.593Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537, Main Thread] WARNING: '!obs', file /builds/worker/checkouts/gecko/toolkit/components/sessionstore/RestoreTabContentObserver.cpp:58
[task 2021-04-01T17:11:13.629Z] 17:11:13     INFO - GECKO(1537) | [Parent 1537, Main Thread] WARNING: NS_ENSURE_TRUE(Preferences::InitStaticMembers()) failed: file /builds/worker/checkouts/gecko/modules/libpref/Preferences.cpp:4442
[task 2021-04-01T17:11:13.670Z] 17:11:13     INFO - TEST-INFO | Main app process: exit 0
[task 2021-04-01T17:11:13.670Z] 17:11:13     INFO - TEST-INFO | Confirming we saw 422 DOCSHELL created and 422 destroyed log strings.
[task 2021-04-01T17:11:13.671Z] 17:11:13     INFO - TEST-INFO | Confirming we saw 1253 DOMWINDOW created and 1253 destroyed log strings.
[task 2021-04-01T17:11:13.679Z] 17:11:13    ERROR - TEST-UNEXPECTED-FAIL | remote/cdp/test/browser/page/browser_javascriptDialog_alert.js | leaked 4 window(s) until shutdown [url = about:blank]
[task 2021-04-01T17:11:13.679Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_alert.js | windows(s) leaked: [pid = 1537] [serial = 233], [pid = 1537] [serial = 238], [pid = 1537] [serial = 228], [pid = 1537] [serial = 239]
[task 2021-04-01T17:11:13.681Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_alert.js | This test created 1 hidden window(s)
[task 2021-04-01T17:11:13.681Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_alert.js | This test created 1 hidden docshell(s)
[task 2021-04-01T17:11:13.681Z] 17:11:13    ERROR - TEST-UNEXPECTED-FAIL | remote/cdp/test/browser/page/browser_javascriptDialog_beforeunload.js | leaked 4 window(s) until shutdown [url = about:blank]
[task 2021-04-01T17:11:13.681Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_beforeunload.js | windows(s) leaked: [pid = 1537] [serial = 245], [pid = 1537] [serial = 255], [pid = 1537] [serial = 256], [pid = 1537] [serial = 250]
[task 2021-04-01T17:11:13.682Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_beforeunload.js | This test created 1 hidden window(s)
[task 2021-04-01T17:11:13.683Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_beforeunload.js | This test created 1 hidden docshell(s)
[task 2021-04-01T17:11:13.684Z] 17:11:13    ERROR - TEST-UNEXPECTED-FAIL | remote/cdp/test/browser/page/browser_javascriptDialog_confirm.js | leaked 4 window(s) until shutdown [url = about:blank]
[task 2021-04-01T17:11:13.684Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_confirm.js | windows(s) leaked: [pid = 1537] [serial = 262], [pid = 1537] [serial = 267], [pid = 1537] [serial = 272], [pid = 1537] [serial = 273]
[task 2021-04-01T17:11:13.685Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_confirm.js | This test created 1 hidden window(s)
[task 2021-04-01T17:11:13.685Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_confirm.js | This test created 1 hidden docshell(s)
[task 2021-04-01T17:11:13.686Z] 17:11:13    ERROR - TEST-UNEXPECTED-FAIL | remote/cdp/test/browser/page/browser_javascriptDialog_prompt.js | leaked 4 window(s) until shutdown [url = about:blank]
[task 2021-04-01T17:11:13.687Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_prompt.js | windows(s) leaked: [pid = 1537] [serial = 303], [pid = 1537] [serial = 293], [pid = 1537] [serial = 298], [pid = 1537] [serial = 304]
[task 2021-04-01T17:11:13.687Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_prompt.js | This test created 1 hidden window(s)
[task 2021-04-01T17:11:13.688Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_javascriptDialog_prompt.js | This test created 1 hidden docshell(s)
[task 2021-04-01T17:11:13.688Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_bringToFront.js | This test created 1 hidden window(s)
[task 2021-04-01T17:11:13.688Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_bringToFront.js | This test created 1 hidden docshell(s)
[task 2021-04-01T17:11:13.688Z] 17:11:13     INFO - TEST-INFO | remote/cdp/test/browser/page/browser_captureScreenshot.js | This test created 1 hidden window(s)
...
...
...

Hooray, shutdown leak. Investigating.

mccr8 has graciously offered to take a look at these shutdown leaks.

I've looked at this a bit, by logging all CCs and comparing what is holding the window alive before shutdown to what is around when we free it. It appears to be some references to a XULFrameElement (the browser?) that go away in between.

There are a couple of maps in NetworkObserver.jsm, _browserSessionCount and _browserResponseStorages, that are holding references to the browser elements. Maybe these could be weak maps? Maybe there's some other mechanism that is supposed to drop these references that isn't working? It is also possible that there's some issue further upstream that is causing the leak. I tried making most of the maps into weak maps and that wasn't enough to fix the leak. I'll look tomorrow to see if it changed anything.

Thanks, mccr8.

One thing I had a suspicion about last Friday, but didn't get much headway on, is this "preload" dialog: https://searchfox.org/mozilla-central/rev/166dfa51ee50207a253fc577b9a596e64f24258c/toolkit/modules/SubDialog.jsm#867-872

I found that if I gave each SubDialog instance a unique numeric ID, and logged their construction and removal from the DOM, I think the first one never got used... so maybe it sticks around in the DOM somehow... but it's unclear how this patch would have revealed that problem.

I added a new option to my dom_tree.py script to print out the path through the dominator tree for a GC log to a specific object. If there's a path from A to B, that means that all paths from a root that go to B also go through A. This can give you a sense of what the "bottleneck" objects are when trying to figure out how a leak is happening. This might give some more food for thought when figuring out what might be going wrong.

Anyways, here's a dominator tree path to one of the windows: RemoteAgent.jsm object, targetList, EventEmitter.jsm object, a _eventEmitterListeners map, Domain.jsm object, TabSession object, browser (XULFrameElement), TabDialogBox object, SubDialogManager object, SubDialog object, _frameCreated promise, an event, then finally the leaking window.

I do see some dialog stuff in there.

Right, that's consistent with what I observed, which is that if I replace this line of code here: https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34cb178960f7375210a0bba4/toolkit/modules/SubDialog.jsm#56-61

with this:

  this._frameCreated = new Promise(resolve => {
    this._frame.addEventListener("load", () => {
      resolve();
    }, {
      once: true,
      capture: true,
    });
  });

then our leak goes away. I guess this is because the event (the second last item in your list) no longer gets passed to the Promise resolver, so stops leaking the window.

Like, we could do this and move on... but what's actually keeping that event alive? Does https://searchfox.org/mozilla-central/rev/ee9dab6aa95f167a34cb178960f7375210a0bba4/toolkit/modules/SubDialog.jsm#137 never resolve or something, so this scope stays alive somehow in the task queue?

The event is being held in **UNKNOWN SLOT 1** of the Promise, which yeah does sound like the promise must not be getting resolved.

_frameCreated is being held alive by the SubDialog, so if we never get to the await, then it'll be kept alive.

Hey pbz,

Can you think of a reason why this patch would cause a SubDialog instance (preloadDialog maybe?) to stay alive? Do we do any work to clean up any preloadDialog SubDialogs at window unload time? I see that we do it on TabClose, but recall that TabClose doesn't fire when windows themselves close...

TabDialogBox does some cleanup on TabClose, but that doesn't affect the preload dialog, because that's not part of SubDialogManager._dialogs. Apart from that, I don't think we do any explicit cleanup.

SubDialogManager always holds a preload dialog for the next open call. It creates one in the constructor and after each open call. This is still from the old preferences subdialog.js code (It might be worth investigating whether we still need this code and preloading SubDialogs really helps with performance).

(In reply to Andrew McCreight [:mccr8] from comment #13)

_frameCreated is being held alive by the SubDialog, so if we never get to the await, then it'll be kept alive.

This seems likely. Unused preload dialogs will never get an open call and thus we'll never consume that promise. Though I'm wondering why we're running into this issue just now. If that's the issue I don't think we need to clean up explicitly. mconley's solution from comment 11 seems good.

mconley's solution from comment 11 seems good.

Looks like I just found a reviewer. Okay, thanks!

And thanks mccr8 for the investigation. I can take it from here, I think.

Depends on D110270

https://hg.mozilla.org/mozilla-central/rev/da294b91b46c
https://hg.mozilla.org/mozilla-central/rev/23c50771512e