One of the test fixes from bug 1397365 involves removing the `nodefaultsrc` attribute when flipping browser remoteness in updateBrowserRemoteness. The patch looks like the attachment. The consequence of the patch would be that whenever we flip the remoteness of the initial browser, we load about:blank in the newly (re-)inserted browser, except when the magical flag is passed, which only happens in the initial updating of remoteness for that browser. This is strictly better than it is today (ie fewer cases where we load about:blank), so I mean, that's fine, and I could fold that into changes to land in bug 1362774 or separately (maybe here, if we don't figure out we want to do something else). However, I have a few concerns with the approach: - all of this seems to be about the allowscriptstoclose webextension flag and communicating that to the child. Given the actor-based changes we've made recently, is there no better way to implement that feature that doesn't rely on whether or not we create about:blank documents? - although we're opting out of this behavior on the initial remoteness flip, any further remoteness flips on the initial browser would end up with a spurious about:blank load. This can happen with webextension-based homepages, but also with ones from file:/// or anything else that gets its own content process. That seems unfortunate - not just for the possible perf impact, but also because we end up with a subtly different codepath that is barely if at all exercised in automated tests, but will happen for some users. I'm worried that will end up being a source of hard-to-track-down bugs. - on top of the previous point, if we at any time remove the attribute, the attribute never gets re-added on future remoteness flips, so the initial browser remains forever "crippled" in this way, creating about:blank docs whenever its remoteness changes (which potentially will happen more often once we put about:home/about:newtab in its own process). - in the opposite direction, why is this necessary for the initial tab and not the other tabs? In https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/browser/components/extensions/parent/ext-windows.js#216-222 it seems we want this change to affect all tabs created with the window. It looks like window.create supports being called with more than 1 URL, and thus it will try to open more than one tab, and this change should affect all the tabs, but AFAICT any tab other than the initial tab would be broken right now (because we already always set nodefaultsrc on the non-initial tabs and never remove it, AIUI). - at least for me and mconley, running the test locally on its own already breaks (with vanilla m-c); (I tested on a 15" late 2017 mbp running macOS). AFAICT the test is also hardcoded to run with in-process webextensions, so I don't know how that affects things. So given all of that, I think that we may need a different solution here. Kris, can you answer some of the questions in the above, and/or suggest an alternative way of addressing this problem? I'm happy to do the work, but I'd like to make sure I understand the goal of the code here, and your concern in bug 1397365 comment 75 that `removing "nodefaultsrc" [is] not clearly the correct solution` before embarking on something else.

(In reply to :Gijs (he/him) from comment #0) > Kris, can you answer some of the questions in the above, and/or suggest an > alternative way of addressing this problem? I'm happy to do the work, but > I'd like to make sure I understand the goal of the code here, and your > concern in bug 1397365 comment 75 that `removing "nodefaultsrc" [is] not > clearly the correct solution` before embarking on something else. Egh, that was meant to be: `removing "nodefaultsrc" [is] clearly the correct solution` (so one fewer negation...)

(In reply to :Gijs (he/him) from comment #0) > - all of this seems to be about the allowscriptstoclose webextension flag > and communicating that to the child. Given the actor-based changes we've > made recently, is there no better way to implement that feature that doesn't > rely on whether or not we create about:blank documents? I'm not entirely sure why creating about:blank documents affects this? > - in the opposite direction, why is this necessary for the initial tab and > not the other tabs? Because the intent of this is to allow scripts to close the initial tab of a new window, in the same way as window.open() would allow scripts in that window to close only the (inner, content) window which was created by window.open. That flag isn't meant to be propagated to other tabs which happen to be in the same browser window as them. I'm not particularly concerned about the flag persisting after navigations or remoteness flips. It just needs to apply to the document which was initially loaded by browser.windows.create() > In > https://dxr.mozilla.org/mozilla-central/rev/c2e3be6a1dd352b969a45f0b85e87674e24ad284/browser/components/extensions/parent/ext-windows.js#216-222 > it seems we want this change to affect all tabs created with the window. It > looks like window.create supports being called with more than 1 URL, and thus > it will try to open more than one tab, and this change should affect all the > tabs, but AFAICT any tab other than the initial tab would be broken right now > (because we already always set nodefaultsrc on the non-initial tabs and never > remove it, AIUI). In theory, I suppose we do. In practice, I don't think that's a use case anyone cares about. > - at least for me and mconley, running the test locally on its own already > breaks (with vanilla m-c); (I tested on a 15" late 2017 mbp running macOS). > AFAICT the test is also hardcoded to run with in-process webextensions, so I > don't know how that affects things. Yeah, the whole current setup is a nightmare. We need something better.

(In reply to Kris Maglione [:kmag] from comment #2) > (In reply to :Gijs (he/him) from comment #0) > > - all of this seems to be about the allowscriptstoclose webextension flag > > and communicating that to the child. Given the actor-based changes we've > > made recently, is there no better way to implement that feature that doesn't > > rely on whether or not we create about:blank documents? > > I'm not entirely sure why creating about:blank documents affects this? I assume this just means there's more time for the parent to send the right message to the child, but it's kinda hard to debug given that the test fails without the changes from bug 1362774 when run in isolation anyway. I figure if I fix that, that'll probably fix the issues that happen on infra (in run-by-dir) when the changes from bug 1362774 are included, too. So, fwiw, I dug into this a bunch today, and I at least know the following: - XULFrameLoaderCreated is no guarantee that we have a message manager. Specifically, that event gets fired by the XUL frame element ( https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/xul/XULFrameElement.cpp#109 ) once we start a load... but just creating the frame loader doesn't create a message manager. Specifically, that won't happen until someone calls `ReallyLoadFrameScripts()` on the frameloader (generally from TryRemoteBrowser()), causing it to call InitWithCallback() on the message manager, which passes the frameloader as the callback object for the message manager. - adding an event for the message manager being available, and listening to that instead of XULFrameLoaderCreated, with the listener added in the "load" handler, unbreaks the initial test for the moz-extension: URI, but doesn't unbreak explicit passing of AllowScriptsToClose and loading example.com instead. Feels like there's still some other race condition happening, and I'm not sure yet what that is. In terms of having better architecture here... we need "something" to tell the child process docshell that scripts are allowed to close it. I'm pretty tempted to add code like this: https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/base/nsFrameLoader.cpp#2705-2714 so that we can just set an attribute on the browser and propagate that down to content in that same place. Kris/Nika, how would you feel about that? Is there a better way to do this?

(In reply to :Gijs (he/him) from comment #3) > (In reply to Kris Maglione [:kmag] from comment #2) > > (In reply to :Gijs (he/him) from comment #0) > > > - all of this seems to be about the allowscriptstoclose webextension flag > > > and communicating that to the child. Given the actor-based changes we've > > > made recently, is there no better way to implement that feature that doesn't > > > rely on whether or not we create about:blank documents? > > > > I'm not entirely sure why creating about:blank documents affects this? > > I assume this just means there's more time for the parent to send the right > message to the child, but it's kinda hard to debug given that the test fails > without the changes from bug 1362774 when run in isolation anyway. I figure > if I fix that, that'll probably fix the issues that happen on infra (in > run-by-dir) when the changes from bug 1362774 are included, too. > > > So, fwiw, I dug into this a bunch today, and I at least know the following: > > - XULFrameLoaderCreated is no guarantee that we have a message manager. > Specifically, that event gets fired by the XUL frame element ( > https://searchfox.org/mozilla-central/rev/ > de7676288a78b70d2b9927c79493adbf294faad5/dom/xul/XULFrameElement.cpp#109 ) > once we start a load... but just creating the frame loader doesn't create a > message manager. Err, rather, doesn't immediately have a *functional* message manager - the message manager doesn't let you use it until mCallback is non-null, and so calling sendAsyncMessage fails with NS_ERROR_NOT_INITIALIZED - https://dxr.mozilla.org/mozilla-central/rev/703546ab6d0cb643028a1ab4fda997b38f38a2e6/dom/base/nsFrameMessageManager.cpp#596

(In reply to :Gijs (he/him) from comment #3) > - XULFrameLoaderCreated is no guarantee that we have a message manager. > Specifically, that event gets fired by the XUL frame element ( > https://searchfox.org/mozilla-central/rev/ > de7676288a78b70d2b9927c79493adbf294faad5/dom/xul/XULFrameElement.cpp#109 ) > once we start a load... but just creating the frame loader doesn't create a > message manager. Specifically, that won't happen until someone calls > `ReallyLoadFrameScripts()` on the frameloader (generally from > TryRemoteBrowser()), causing it to call InitWithCallback() on the message > manager, which passes the frameloader as the callback object for the message > manager. Well, that sounds like a recipe for trouble. There are a bunch of other places we rely on this assumption too... > In terms of having better architecture here... we need "something" to tell > the child process docshell that scripts are allowed to close it. > > I'm pretty tempted to add code like this: > > https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/base/nsFrameLoader.cpp#2705-2714 > > so that we can just set an attribute on the browser and propagate that down > to content in that same place. > > Kris/Nika, how would you feel about that? Yes.

Kinda assuming resolving this wasn't intentional - I'd like to use it for a patch for the allowscriptstoclose thing. I'll file a follow-up to audit other XULFrameLoaderCreated callsites, assuming Nika or someone else can confirm my read of the code...

I just have a review request for this now, so clearing ni. Note that I didn't fix up all the webextensions uses of this. I'll file a follow-up bug if we stick with this approach, because AFAICT we use the same messaging stuff elsewhere (e.g. popups and stuff?) and we could switch that over separately if/when this lands. Maybe we can drop the domwindowutils helper (which just calls the outer window method) then as well.

Comment on attachment 9008771 [details] Bug 1488822 - propagate allowScriptsToClose via the frameloader instead of relying on frame scripts, r?mconley!,nika!,kmag! Kris Maglione [:kmag] has approved the revision.

Comment on attachment 9008771 [details] Bug 1488822 - propagate allowScriptsToClose via the frameloader instead of relying on frame scripts, r?mconley!,nika!,kmag! Mike Conley (:mconley) (:⚙️) has approved the revision.

Comment on attachment 9008771 [details] Bug 1488822 - propagate allowScriptsToClose via the frameloader instead of relying on frame scripts, r?mconley!,nika!,kmag! :Nika Layzell has approved the revision.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a8dd5d2c8cff propagate allowScriptsToClose via the frameloader instead of relying on frame scripts, r=mconley,kmag,nika