1218080 - Fix service workers openWindow when there's no available browser window

Status: RESOLVED FIXED

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

Description

[Cătălin Badea (:catalinb)]

This is a follow-up for bug 1172870 to address the case where firefox is running without a browser window.

Comment 1

[Cătălin Badea (:catalinb)]

We can run into this bug more often than previously thought. With an active private browsing window, firefox can receive notifications and run service workers but won't have a valid browser window for openWindow.

Comment 2

[Cătălin Badea (:catalinb)]

Attachment: Use the hidden XUL window when no browser window is available for ServiceWorkerClients.openWindow.

Comment 3

[Cătălin Badea (:catalinb)]

Attachment: Don't send the url to the parent process when opening new windows, because it is not actually used.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8744901 [details] [diff] [review]
Use the hidden XUL window when no browser window is available for ServiceWorkerClients.openWindow.

So I think we should somehow explicitly tell when it is ok to not have tabparent when sending BrowserFrameOpenWindow message from child side, and only use that for sw.openWindow case, at least for now.
Should be a simple change, but could take a look at another patch.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8744902 [details] [diff] [review]
Don't send the url to the parent process when opening new windows, because it is not actually used.

Fun. nsWindowWatcher code is so great :/
Could we add an assertion to nsWindowWatcher::OpenWindowInternal that if aOpeningTab is non-null, aURL is null.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

mike, you were thinking about cleaning up WW code. See the latter patch here as another example of the messiness of the code :)

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Youch. Thanks for cleaning that up, catalinb.

Comment 8

[Cătălin Badea (:catalinb)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8744901 [details] [diff] [review]
> Use the hidden XUL window when no browser window is available for
> ServiceWorkerClients.openWindow.
> 
> So I think we should somehow explicitly tell when it is ok to not have
> tabparent when sending BrowserFrameOpenWindow message from child side, and
> only use that for sw.openWindow case, at least for now.
> Should be a simple change, but could take a look at another patch.

I'm not sure I follow. Service Workers use the SendCreateWindow path. As far as I know, BrowserFrameOpenWindow is used only on Firefox OS and always expects a valid tabchild, which is enforced through asserts.

Are you suggesting adding a flag that explicitly marks that (in the child) the call is coming from a sw instead of guessing based on whether aTabOpener is null or not?

Comment 9

[Cătălin Badea (:catalinb)]

Attachment: Don't send the url to the parent process when opening new windows, because it is not actually used.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13c042a06d5c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13c042a06d5c

Comment 12

[Cătălin Badea (:catalinb)]

This was fixed when the openWindow code was refactored with the rest of the API for clients.

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

This is not fixed, we certainly has a TODO comment referring to this bug https://searchfox.org/mozilla-central/rev/f571db8014431de31d245017e2f5457046aec4ea/dom/clients/manager/ClientOpenWindowUtils.cpp#222

  // Find the most recent browser window and open a new tab in it.
  nsCOMPtr<nsPIDOMWindowOuter> browserWindow =
      nsContentUtils::GetMostRecentNonPBWindow();
  if (!browserWindow) {
    // It is possible to be running without a browser window on Mac OS, so
    // we need to open a new chrome window.
    // TODO(catalinb): open new chrome window. Bug 1218080
    aRv.ThrowTypeError("Unable to open window");
    return;
  }

Comment 15

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1218080 - Part 2: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth

Comment 16

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1218080 - Part 3: Add mochitest for referrer from clients.openWindow r=asuth

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: Bug 1218080 - Part 1: Add class info for nsOpenWindowInfo r=asuth

This allows nsOpenWindowInfo to be automatically detected as nsIOpenWindowInfo from JS without extra QueryInterface.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b600abae15fe
https://hg.mozilla.org/integration/autoland/rev/91d1452525c3
Part 3: Add mochitest for referrer from clients.openWindow r=asuth

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/91d1452525c3

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/09a47f313485
https://hg.mozilla.org/integration/autoland/rev/9c91f34a4087
Part 1: Add class info for nsOpenWindowInfo r=asuth
https://github.com/mozilla-firefox/firefox/commit/83049527d48a
https://hg.mozilla.org/integration/autoland/rev/bb69930c46e2
Part 2: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bc07706a80df
https://hg.mozilla.org/integration/autoland/rev/dfc51798a6ad
Revert "Bug 1218080 - Part 2: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth" for causing a spike in wpt crashes on windows.

Comment 22

[agoloman]

Backed out for causing a spike in wpt crashes on windows.

• Backout link
• Push with failures
• Failure Log @backdrop-invalidation.html
• Failure Log @svglength-animation-values.html
• Failure Log @018.html?wss

Comment 23

[Kagami Rosylight [:saschanaz] (they/them)]

But those tests have nothing to do with my patch?

Comment 24

[Cosmin Sabou [:CosminS]]

They do not look directly related but on tree we have a bunch of new failures lines with TEST-UNEXPECTED-CRASH since those changes. Might be something from the service workers changes that is triggering them.
You can clearly see the spike in oranges from this link.

Comment 25

[Kagami Rosylight [:saschanaz] (they/them)]

Yeah but I'm confused, this function would only called by parent process, not the content process, and only by service workers and notifications. And it's the content process that is crashing in tests that have nothing to do with service worker nor notification. Actually WPT doesn't even cover that part at all at this point, because we just don't have test infra to do that.

The fact that there's no crash stack confuses me even more.

Andrew, do you see anything obvious that can cause crash here?

Comment 26

[Kagami Rosylight [:saschanaz] (they/them)]

This was the try run before landing https://treeherder.mozilla.org/jobs?repo=try&revision=4775d33ec3383daeea256707e2667b70fa818b06, I tried adding WPT runs there.

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

I added a ton of wpt tests to the linux jobs on the try run too in the hope that they reproduce there so we can use rr/pernosco. I'll take another look when they're done. (Aside: the treeherder auth mechanism has a real problem where treeherder thinks I'm logged in but whatever auth job-triggering has does not and it goes on a tab-opening spree.)

Comment 28

[Kagami Rosylight [:saschanaz] (they/them)]

Also can see Linux failure here: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=bb69930c46e26e677f9b857f5494ee1f0602bdf0&selectedTaskRun=Lh9xOTRLSn2b84q_mZvjCQ.0

But Pernosco has never sent any resulting email to me recently when I trigger it, so maybe you have better chance...

Comment 29

[Kagami Rosylight [:saschanaz] (they/them)]

Wait, those Linux failures are all general non-crash failures??

Comment 30

[Kagami Rosylight [:saschanaz] (they/them)]

Submitted only with the part 1 patch, as I have no idea how part 2 can affect WPT. https://treeherder.mozilla.org/jobs?repo=try&revision=f8699dd969cf58b26d1b1c0a255743ccb317accd

Comment 31

[Kagami Rosylight [:saschanaz] (they/them)]

Ok, this is the try run with only part 1 with full WPT jobs: https://treeherder.mozilla.org/jobs?repo=try&revision=8b79b0c3d5ffb39426dca143a20582461838d90c

So many failures. Seems part 1 is the problem.

Comment 32

[Kagami Rosylight [:saschanaz] (they/them)]

This is an interesting Android crash that is fortunately with an actual crash stack: https://treeherder.mozilla.org/logviewer?job_id=513686227&repo=try&lineNumber=4326

[task 2025-06-18T01:15:46.630+00:00] 01:15:46 INFO - PROCESS-CRASH | MOZ_ASSERT(false) (GeckoView should use nsIBrowserDOMWindow instead) [@ nsWindowWatcher::OpenWindowWithRemoteTab] | /html/browsers/the-window-object/open-close/open-features-tokenization-noreferrer.html

[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - Thread 11 Gecko (crashed) - tid: 12150
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - 0 libxul.so!MOZ_CrashSequence(void*, long) [Assertions.h:8b79b0c3d5ffb39426dca143a20582461838d90c : 248]
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - Found by: inlining
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - 1 libxul.so!nsWindowWatcher::OpenWindowWithRemoteTab(nsIRemoteTab*, mozilla::dom::WindowFeatures const&, mozilla::dom::UserActivation::Modifiers const&, bool, float, nsIOpenWindowInfo*, nsIRemoteTab**) [nsWindowWatcher.cpp:8b79b0c3d5ffb39426dca143a20582461838d90c : 487 + 0x5]
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rax = 0x0000000000000000 rdx = 0x0000000000000001
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rcx = 0x00000000000001e7 rbx = 0x0000000000000001
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rsi = 0x0000070fbb180395 rdi = 0x0000775272cefdd4
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rbp = 0x00007752603647b0 rsp = 0x00007752603647b0
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r8 = 0x0000000000002f76 r9 = 0x00007752603c6450
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r10 = 0x0000000000000000 r11 = 0x0000000000000246
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r12 = 0x0000000000000000 r13 = 0x0000775260364a10
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r14 = 0x000077525f845b88 r15 = 0x000077523c4c5f98
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rip = 0x0000775258b86d64
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - Found by: given as instruction pointer in context
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - 2 libxul.so!mozilla::dom::ContentParent::CommonCreateWindow(mozilla::dom::PBrowserParent*, mozilla::dom::BrowsingContext&, bool, unsigned int const&, bool const&, bool const&, bool const&, bool const&, nsIURI*, nsTSubstring<char> const&, mozilla::dom::UserActivation::Modifiers const&, mozilla::dom::BrowserParent*, nsTSubstring<char16_t> const&, nsresult&, nsCOMPtr<nsIRemoteTab>&, bool*, int&, nsIPrincipal*, nsIReferrerInfo*, bool, nsIContentSecurityPolicy*, mozilla::OriginAttributes const&, bool, bool) [ContentParent.cpp:8b79b0c3d5ffb39426dca143a20582461838d90c : 5323 + 0x25]
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rbx = 0x0000000000000001 rbp = 0x0000775260364940
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rsp = 0x00007752603647c0 r12 = 0x0000000000000000
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r13 = 0x0000775260364a10 r14 = 0x000077525f845b88
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r15 = 0x000077523c4c5f98 rip = 0x0000775257522a92
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - Found by: call frame info
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - 3 libxul.so!mozilla::dom::ContentParent::RecvCreateWindowInDifferentProcess(mozilla::dom::PBrowserParent*, mozilla::dom::MaybeDiscarded<mozilla::dom::BrowsingContext> const&, unsigned int const&, bool const&, bool const&, nsIURI*, nsTSubstring<char> const&, mozilla::dom::UserActivation::Modifiers const&, nsTSubstring<char16_t> const&, nsIPrincipal*, nsIContentSecurityPolicy*, nsIReferrerInfo*, mozilla::OriginAttributes const&, bool, bool) [ContentParent.cpp:8b79b0c3d5ffb39426dca143a20582461838d90c : 5560 + 0x4a]
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rbx = 0x000077523be1fc00 rbp = 0x0000775260364b30
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rsp = 0x0000775260364950 r12 = 0x000077523dc90e00
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r13 = 0x0000775260364d88 r14 = 0x0000775260364a10
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r15 = 0x0000775260364cd9 rip = 0x00007752575245fd
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - Found by: call frame info
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - 4 libxul.so!mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) [PContentParent.cpp: : 10736 + 0x68]
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rbx = 0x000077523dc90e00 rbp = 0x0000775260365dc0
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rsp = 0x0000775260364b40 r12 = 0x0000775260364d50
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r13 = 0x0000775260364db8 r14 = 0x0000775260365d28
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r15 = 0x0000775260365c18 rip = 0x00007752576412b7
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - Found by: call frame info
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - 5 libxul.so!mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) [MessageChannel.cpp:8b79b0c3d5ffb39426dca143a20582461838d90c : 1795 + 0x8]
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rbx = 0x0000775242d3d670 rbp = 0x0000775260365e30
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - rsp = 0x0000775260365dd0 r12 = 0x000077523e960ac0
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r13 = 0x0000000000000001 r14 = 0x000077523dc90e90
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - r15 = 0x0000000000000001 rip = 0x0000775253975e89
[task 2025-06-18T01:15:46.631+00:00] 01:15:46 INFO - Found by: call frame info

Comment 33

[Kagami Rosylight [:saschanaz] (they/them)]

That wasn't reproducible locally though. With /navigation-api/navigate-event/cross-window/location-samedocument-sameorigin.html I can repro 100% but I'm not getting a human readable crash stack. 🤔

Comment 34

[Kagami Rosylight [:saschanaz] (they/them)]

As I first guessed, I think this is not me. /navigation-api/navigate-event/cross-window/location-samedocument-sameorigin.html already crashes on the main branch when ran alone.

Comment 35

[Kagami Rosylight [:saschanaz] (they/them)]

That was a red herring. I can't repro this locally... Another try push with latest main branch: https://treeherder.mozilla.org/jobs?repo=try&revision=ba273e7229a85e679838d74afde27c06512de1b8

Comment 36

[Kagami Rosylight [:saschanaz] (they/them)]

Comment #32 is related to bug 1941334. (Thanks Andrew)

Comment 37

[Kagami Rosylight [:saschanaz] (they/them)]

(In reply to Kagami Rosylight [:saschanaz] (they/them) from comment #35)

That was a red herring. I can't repro this locally... Another try push with latest main branch: https://treeherder.mozilla.org/jobs?repo=try&revision=ba273e7229a85e679838d74afde27c06512de1b8

At this point I wonder this is a compiler issue, as the crash only happens on Windows opt build 🤔

Another run for all platforms on today's main branch: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=138114

Comment 38

[Kagami Rosylight [:saschanaz] (they/them)]

Slight noop change to the patch removed the crashes. 🤷🏻‍♀️ https://treeherder.mozilla.org/jobs?repo=try&revision=29c9f47d10d46a9ecacd75986fa6277610270f94

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/187d85eb5b29
https://hg.mozilla.org/integration/autoland/rev/edbc016a6ba6
Part 1: Add class info for nsOpenWindowInfo r=asuth
https://github.com/mozilla-firefox/firefox/commit/2c4602a6676e
https://hg.mozilla.org/integration/autoland/rev/00a068fc5ed7
Part 2: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth

Comment 40

[amarc]

https://hg.mozilla.org/mozilla-central/rev/edbc016a6ba6
https://hg.mozilla.org/mozilla-central/rev/00a068fc5ed7

Comment 41

[Kagami Rosylight [:saschanaz] (they/them)]

🎉