1622749 - Crash in [@ mozilla::dom::ClientOpenWindow]

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Pascal Chevrel:pascalc]

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

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Chris Peterson [:cpeterson]]

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.

Comment 3

[Jens Stutte [:jstutte]]

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

Comment 4

[Nika Layzell [:nika] (ni? for response)]

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).

Comment 5

[u608768]

Attachment: Bug 1622749 - Run test_notification_openWindow.html with all possible values of browser.link.open_newwindow, r=nika!

Comment 6

[u608768]

(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.

Comment 7

[[:philipp]]

[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?

Comment 8

[Chris Peterson [:cpeterson]]

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.

Comment 9

[Anny Gakhokidze [:annyG]]

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.

Comment 10

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1622749 - Force new documents resulting from clicking on notifications to open in tabs so we don't crash, r=nika!

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c31fd41dd9e
Force new documents resulting from clicking on notifications to open in tabs so we don't crash, r=nika

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3de11b0a07fa
Run test_notification_openWindow.html with all possible values of browser.link.open_newwindow, r=nika

Comment 13

[Cristina Coroiu [:ccoroiu]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Anny Gakhokidze [:annyG]]

Attachment: beta uplift approval request

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

Comment 16

[Anny Gakhokidze [:annyG]]

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

Comment 17

[Anny Gakhokidze [:annyG]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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