Closed Bug 1218080 Opened 10 years ago Closed 1 year ago

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

Categories

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

Product:
Core
Component:
DOM: Service Workers
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox44 --- wontfix

People

(Reporter: catalinb, Assigned: saschanaz)

References

Details

Attachments

(5 files, 1 obsolete file)

Use the hidden XUL window when no browser window is available for ServiceWorkerClients.openWindow.
6.60 KB, patch
smaug
: review-
Details | Diff | Splinter Review
Don't send the url to the parent process when opening new windows, because it is not actually used.
9.44 KB, patch
Details | Diff | Splinter Review
Bug 1218080 - Part 2: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth
48 bytes, text/x-phabricator-request
Details | Review
Bug 1218080 - Part 3: Add mochitest for referrer from clients.openWindow r=asuth
48 bytes, text/x-phabricator-request
Details | Review
Bug 1218080 - Part 1: Add class info for nsOpenWindowInfo r=asuth
48 bytes, text/x-phabricator-request
Details | Review
This is a follow-up for bug 1172870 to address the case where firefox is running without a browser window.
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.
Summary: Fix service workers openWindow on mac os when there's no available browser window → Fix service workers openWindow when there's no available browser window
Assignee: nobody → catalin.badea392
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.
Attachment #8744901 - Flags: review?(bugs) → review-
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.
Attachment #8744902 - Flags: review?(bugs) → review+
mike, you were thinking about cleaning up WW code. See the latter patch here as another example of the messiness of the code :)
Flags: needinfo?(mconley)
Youch. Thanks for cleaning that up, catalinb.
Flags: needinfo?(mconley)
(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?
Keywords: leave-open
Priority: -- → P3
This was fixed when the openWindow code was refactored with the rest of the API for clients.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME

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;
  }
Assignee: catalin.badea392 → krosylight
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Attachment #9489721 - Attachment description: WIP: Bug 1218080 → WIP: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists
Attachment #9489721 - Attachment description: WIP: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists → WIP: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gjis,asuth
Attachment #9489721 - Attachment description: WIP: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gjis,asuth → Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth
Attachment #9489721 - Attachment description: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth → WIP: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth
Attachment #9489721 - Attachment description: WIP: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth → Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth
Attachment #9489721 - Attachment description: Bug 1218080 - Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth → Bug 1218080 - Part 1: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth

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

Attachment #9489721 - Attachment description: Bug 1218080 - Part 1: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth → Bug 1218080 - Part 2: Open a new chrome window on clients.openWindow when no window exists r=Gijs,asuth
Attachment #9492516 - Attachment description: Bug 1218080 - Part 2: Add mochitest for referrer from clients.openWindow r=asuth → Bug 1218080 - Part 3: Add mochitest for referrer from clients.openWindow r=asuth
Pushed by agoloman@mozilla.com: 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.

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

Flags: needinfo?(krosylight)

But those tests have nothing to do with my patch?

Flags: needinfo?(krosylight)

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.

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?

Flags: needinfo?(bugmail)

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

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

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

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. πŸ€”

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.

(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

Slight noop change to the patch removed the crashes. πŸ€·πŸ»β€β™€οΈ https://treeherder.mozilla.org/jobs?repo=try&revision=29c9f47d10d46a9ecacd75986fa6277610270f94

Flags: needinfo?(bugmail)

πŸŽ‰

Status: REOPENED → RESOLVED
Closed: 8 years ago → 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: