This bug is for crash report bp-c7d6c59b-46c3-4a74-b1a7-9647a0200314.

Top 10 frames of crashing thread:

0 xul.dll mozilla::dom::ClientOpenWindow dom/clients/manager/ClientOpenWindowUtils.cpp:401
1 xul.dll mozilla::detail::ProxyFunctionRunnable<`lambda at Z:/task_1584182567/build/src/dom/clients/manager/ClientManagerService.cpp:568:22', mozilla::MozPromise<mozilla::dom::ClientOpResult, mozilla::CopyableErrorResult, 0> >::Run xpcom/threads/MozPromise.h:1456
2 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1220
3 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:481
4 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
5 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
6 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
7 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137
8 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:406
9 xul.dll nsAppStartup::Run toolkit/components/startup/nsAppStartup.cpp:271

The crash reason is: MOZ_DIAGNOSTIC_ASSERT(bc). This assertion was added (though modified from a similar existing assert) in bug 1578070.

The crash first showed up in the 20200311163942 build. The change sets added in that build are: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=884162af76f5225bbf4efe486959d2fa9757bc56&tochange=4c982daa151954c59f20a9b9ac805c1768a350c2

Bug 1578070 is in there, so I'm going to guess it is a regression from that. Kashav, could you take a look? Thanks.

Moving back to the "DOM: Service Workers" component because this is a Service Workers bug.

Nika says the MOZ_DIAGNOSTIC_ASSERT(bc) assertion is wrong.

needinfo'ing Nika because she will provide the details

Tracking for Fission dogfooding (M5) this is a recent crash regression.

OK, in the meantime I see no more crashes with newer Nightlys. Waiting for Nika's details then.

I'm guessing that this issue is caused by people who have configured their browser not to open new windows within new tabs, but rather within a pop-up.

When calling the nsIBrowserDOMWindow::OpenURI method (https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/dom/clients/manager/ClientOpenWindowUtils.cpp#243-244), the OPEN_DEFAULTWINDOW argument is passed. This is then replaced with a value read from a user's prefs within nsBrowserAccess here: https://searchfox.org/mozilla-central/rev/3fd53c47864fedb916f0ed8f002f15456324f729/browser/base/content/browser.js#6085-6098

If the user has configured their browser to use OPEN_NEWWINDOW, this will call openDialog to open a new browser window, but does not wait for the newly created window to finish loading, so no content window has been created yet at this point, and the frontend JS returns null: https://searchfox.org/mozilla-central/rev/3fd53c47864fedb916f0ed8f002f15456324f729/browser/base/content/browser.js#6146-6156

It is also very possible to get null values from other open codepaths, if the window.open failed, as generally this method doesn't throw an exception.

An easier workaround would be to force OPEN_NEWTAB for the opening strategy, to avoid the issues with OPEN_NEWWINDOW, but in order to fix OPEN_NEWWINDOW as well, other, more complicated, tweaks would need to be made to how we open new windows. Namely, we'd need to make this decision in C++, and call out to other native methods to handle the OPEN_NEWWINDOW case, like we do within ContentParent::CommonCreateWindow (https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/dom/ipc/ContentParent.cpp#4702-4703).

Either way, we should probably run tests for the createWindow API with the browser.link.open_newwindow pref set to 2 (OPEN_NEWWINDOW).

(In reply to :Nika Layzell (ni? for response) from comment #4)

It is also very possible to get null values from other open codepaths, if the window.open failed, as generally this method doesn't throw an exception.

An easier workaround would be to force OPEN_NEWTAB for the opening strategy, to avoid the issues with OPEN_NEWWINDOW, but in order to fix OPEN_NEWWINDOW as well, other, more complicated, tweaks would need to be made to how we open new windows. Namely, we'd need to make this decision in C++, and call out to other native methods to handle the OPEN_NEWWINDOW case, like we do within ContentParent::CommonCreateWindow (https://searchfox.org/mozilla-central/rev/202a285024f174c2d2bf2152d9cba90a03723eab/dom/ipc/ContentParent.cpp#4702-4703).

I can try the latter. I suppose we'll just want to throw a generic "could not open a window" in other cases where we end up with a null browsing context?

Either way, we should probably run tests for the createWindow API with the browser.link.open_newwindow pref set to 2 (OPEN_NEWWINDOW).

I've updated the test from bug 1578070 to run with browser.link.open_newwindow set to both 2 and 3. As expected, it crashes when the pref is set to 2.

[Tracking Requested - why for this release]:
this crash signature is hitting higher numbers after 76 went to the beta population - can we pump up the priority?

Nika's proposed hackaround fix for 76 Beta uplift: override user decision to open links in a new window instead of new tab? Force "always new tab" instead of "new window". See comment 4.

Anny volunteered to work on this bug.

Making this bug P1 for Fission M5a because this crash is affecting non-Fission users in 76 Beta.

I've thought of a plan for this! We can return a promise from JS, and resolve it when we get a browsing context somewhere around the call to openDialog https://searchfox.org/mozilla-central/rev/9120151ddb35f2d4c37bfe613a54a4f10a9a3dc5/browser/base/content/browser.js#6128-6143. ClientOpenWindow() returns a promise so this should theoretically work out as ClientOpenWindow calls dom::OpenWindow (and is the only caller of it), and dom::OpenWindow calls nsBrowserAccess.openURI which calls nsBrowserAccess.getContentWindowOrOpenURI which eventually calls openDialog. Need to think about what the best way to go about this without introducing duplicate functions as nsBrowserAccess.openURI gets called in other places as well.

Stay tuned.

This is a temporary solution as we don't want to permanently override users'
preferences.

https://hg.mozilla.org/mozilla-central/rev/4c31fd41dd9e
https://hg.mozilla.org/mozilla-central/rev/3de11b0a07fa

Please nominate this for Beta uplift when you get a chance.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1578070
[User impact if declined]: When the user receives a browser notification (and pref “browser.link.open_newwindow” is set to 2, meaning new documents will open in a new window) and clicks on it, a crash occurs.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: The change is low risk in the technical sense, as we are only overriding users' preference of opening documents in a new window, to instead open documents in a new tab. This is a temporary change until I write a better solution, but it is better than a crash.
[Why is the change risky/not risky?]:
[String changes made/needed]: None

Keeping it open because I want to use this bug to attach a better solution for this crash.

:RyanVM suggested to open a new bug for the follow up work, so that's what I'll do :)

Comment on attachment 9135169 [details]
Bug 1622749 - Run test_notification_openWindow.html with all possible values of browser.link.open_newwindow, r=nika!

Avoid crashing by opening documents in a new tab. Approved for 76.0b5.

https://hg.mozilla.org/releases/mozilla-beta/rev/2bb367f73d58
https://hg.mozilla.org/releases/mozilla-beta/rev/a3482e46d0b6