Closed Bug 1622749 Opened 9 months ago Closed 8 months ago

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

Categories

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

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla77
Fission Milestone M5b
Tracking Status
firefox-esr68 --- unaffected
firefox74 --- unaffected
firefox75 --- unaffected
firefox76 + fixed
firefox77 + fixed

People

(Reporter: pascalc, Assigned: annyG)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 1 obsolete file)

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

Component: General → DOM: Core & HTML
Component: DOM: Core & HTML → DOM: Service Workers

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.

Flags: needinfo?(kmadan)
Keywords: regression
Regressed by: 1578070
Component: DOM: Service Workers → DOM: Content Processes

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.

Fission Milestone: --- → M5
Component: DOM: Content Processes → DOM: Service Workers
Flags: needinfo?(nika)

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

Priority: -- → P3

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

Flags: needinfo?(nika)

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

Assignee: nobody → kmadan
Flags: needinfo?(kmadan)

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

Severity: normal → critical
Priority: P3 → --

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.

Assignee: kmadan → agakhokidze
Fission Milestone: M5 → M5b
Priority: -- → P1

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.

Attachment #9135169 - Attachment description: Bug 1622749 - Run test_notification_openWindow.html with all possible values of browser.link.open_newwindow → Bug 1622749 - Run test_notification_openWindow.html with all possible values of browser.link.open_newwindow, r=nika!
Pushed by agakhokidze@mozilla.com:
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
Pushed by agakhokidze@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3de11b0a07fa
Run test_notification_openWindow.html with all possible values of browser.link.open_newwindow, r=nika
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77

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

Flags: needinfo?(agakhokidze)
Attached file beta uplift approval request (obsolete) —

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

Flags: needinfo?(agakhokidze)
Attachment #9140802 - Flags: approval-mozilla-beta?
Keywords: leave-open

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

Keywords: leave-open
Regressions: 1630323
Attachment #9140802 - Flags: approval-mozilla-beta?
Attachment #9140802 - Attachment is obsolete: true

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.

Attachment #9135169 - Flags: approval-mozilla-beta+
Attachment #9140263 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.