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

RESOLVED WORKSFORME

Status

()

P3
normal
RESOLVED WORKSFORME
3 years ago
6 months ago

People

(Reporter: catalinb, Assigned: catalinb)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox44 wontfix)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

3 years ago
status-firefox44: affected → wontfix
(Assignee)

Comment 1

3 years ago
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)

Updated

3 years ago
Assignee: nobody → catalin.badea392
(Assignee)

Comment 2

3 years ago
Created attachment 8744901 [details] [diff] [review]
Use the hidden XUL window when no browser window is available for ServiceWorkerClients.openWindow.
Attachment #8744901 - Flags: review?(bugs)
(Assignee)

Comment 3

3 years ago
Created attachment 8744902 [details] [diff] [review]
Don't send the url to the parent process when opening new windows, because it is not actually used.
Attachment #8744902 - Flags: review?(bugs)

Comment 4

3 years ago
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 5

3 years ago
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+

Comment 6

3 years ago
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)
(Assignee)

Comment 8

3 years ago
(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?
(Assignee)

Comment 9

3 years ago
Created attachment 8748186 [details] [diff] [review]
Don't send the url to the parent process when opening new windows, because it is not actually used.
Attachment #8744902 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: leave-open

Updated

9 months ago
Priority: -- → P3
(Assignee)

Comment 12

6 months ago
This was fixed when the openWindow code was refactored with the rest of the API for clients.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → WORKSFORME
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.