Closed Bug 1316659 Opened 8 years ago Closed 7 years ago

Store strong refs to pending openWindow operations in SWP

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox54 --- fixed

People

(Reporter: catalinb, Assigned: catalinb)

Details

Attachments

(2 files, 5 obsolete files)

At http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.h#229

we keep raw pointers to the pending openWindow runnables, which is bad.
This patch also changes the order in which pending openWindow operations
are executed.
Attachment #8809503 - Flags: review?(bugmail)
Comment on attachment 8809503 [details] [diff] [review]
Use strong refs for pending openWindow operations.

Review of attachment 8809503 [details] [diff] [review]:
-----------------------------------------------------------------

NB: I realize most of this is pre-existing code.

I'm confused about how the observer registration works at all.  We're adding ServiceWorkerPrivate as the observer with ownsWeak=true, but ServiceWorkerPrivate doesn't implement nsISupportsWeakReference; ServiceWorkerClients does.  Unless I'm missing something, the call to AddObserver seems like it should be failing to add an observer and returning NS_NOINTERFACE.  Additionally, there's the issue that we may end up adding the ServiceWorkerPrivate as an observer N times if there are N pending open calls.  While the removal logic should handle that/converge, it's probably better to avoid doing that.

I would suggest:
- Move the AddObserver call into AddPendingWindowOp.  Only add the observer if we're transitioning from 0 to 1 present entries.  (The existing/patched remove logic is fine with this.)
- Add a comment to the AddObserver call explaining the rationale for having a weak reference to SWP.  Something like: "If the ServiceWorker is terminated, the runnable's PromiseWorkerProxy will be cleaned up, so there is no point having the observer service keep the ServiceWorkerPrivate and thereby mPendingWindowOps alive since they'll just fail when they run."
- Add a NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference) to ServiceWorkerPrivate.

If you've already addressed this in another patch, please cross-reference it. 

For the specifics of this patch, restating my understanding:
- On Fennec (only), it's possible for there to not be an open browser window but for service worker logic to be active as a result of a desktop notification being clicked, allowing Clients.openWindow() to trigger an attempt to open a window.  In this case OpenWindow() will fail with NS_ERROR_NOT_AVAILABLE and we'll try and use the above-mentioned observer listener for "BrowserChrome:Ready" to wait until Fennec has started up its browser subsystem and they we let the runnables try again.
- Without the patch, once the OpenWindowRunnable::Run call which is invoking AddPendingWindowOp returns, there are no owning references to the runnable held anywhere, and if we attempt to dispatch the freed runnables, horrible things will happen.
- Right now horrible things are probably not happening because of the observer service issue listed above.

I'm giving feedback+ because the change to a strong reference and the name changes look good to me, but I want to make sure that the observer issue is resolved either here or in another bug.  Also, I can only give r+ if you are an authorized reviewer for the code deferring the review to me.  So you still need to ask Ben for the final review or to defer to me :)
Attachment #8809503 - Flags: review?(bugmail) → feedback+
(In reply to Andrew Sutherland [:asuth] from comment #2)
> Comment on attachment 8809503 [details] [diff] [review]
> Use strong refs for pending openWindow operations.
> 
> Review of attachment 8809503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> NB: I realize most of this is pre-existing code.
> 
> I'm confused about how the observer registration works at all.  We're adding
> ServiceWorkerPrivate as the observer with ownsWeak=true, but
> ServiceWorkerPrivate doesn't implement nsISupportsWeakReference;

That's bad, thanks for spotting that!

> ServiceWorkerClients does.  Unless I'm missing something, the call to

ServiceWorkerClients doesn't implement nsISupportsWeakReference, I guess you're referring to
WebProgressListener.

> AddObserver seems like it should be failing to add an observer and returning
> NS_NOINTERFACE.  Additionally, there's the issue that we may end up adding
> the ServiceWorkerPrivate as an observer N times if there are N pending open
> calls.  While the removal logic should handle that/converge, it's probably
> better to avoid doing that.

Yes, without an actual test for this - it's difficult to catch this. There's a bug for this 1313096.
> 
> I would suggest:
> - Move the AddObserver call into AddPendingWindowOp.  Only add the observer
> if we're transitioning from 0 to 1 present entries.  (The existing/patched
> remove logic is fine with this.)
> - Add a comment to the AddObserver call explaining the rationale for having
> a weak reference to SWP.  Something like: "If the ServiceWorker is
> terminated, the runnable's PromiseWorkerProxy will be cleaned up, so there
> is no point having the observer service keep the ServiceWorkerPrivate and
> thereby mPendingWindowOps alive since they'll just fail when they run."
> - Add a NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference) to
> ServiceWorkerPrivate.
> 
> If you've already addressed this in another patch, please cross-reference
> it.
I'll make the changes in this bug. 

> For the specifics of this patch, restating my understanding:
> - On Fennec (only), it's possible for there to not be an open browser window
> but for service worker logic to be active as a result of a desktop
> notification being clicked, allowing Clients.openWindow() to trigger an
> attempt to open a window.  In this case OpenWindow() will fail with
> NS_ERROR_NOT_AVAILABLE and we'll try and use the above-mentioned observer
> listener for "BrowserChrome:Ready" to wait until Fennec has started up its
> browser subsystem and they we let the runnables try again.
> - Without the patch, once the OpenWindowRunnable::Run call which is invoking
> AddPendingWindowOp returns, there are no owning references to the runnable
> held anywhere, and if we attempt to dispatch the freed runnables, horrible
> things will happen.
> - Right now horrible things are probably not happening because of the
> observer service issue listed above.
Sounds about right, not sure if AddObserver fails or not if ServiceWorkerPrivate doesn't
implement weak reference.

> 
> I'm giving feedback+ because the change to a strong reference and the name
> changes look good to me, but I want to make sure that the observer issue is
> resolved either here or in another bug.  Also, I can only give r+ if you are
> an authorized reviewer for the code deferring the review to me.  So you
> still need to ask Ben for the final review or to defer to me :)

How does one become an authorized reviewer? You seem to have a good understanding of this code, so if you can review it, that would be great.
(In reply to Cătălin Badea (:catalinb) from comment #3)
> ServiceWorkerClients doesn't implement nsISupportsWeakReference, I guess
> you're referring to
> WebProgressListener.

Yes, sorry.  I was just going by the grep (or rather the great and powerful "rg!" :).
 
> How does one become an authorized reviewer? You seem to have a good
> understanding of this code, so if you can review it, that would be great.

See https://www.mozilla.org/en-US/about/governance/policies/module-ownership/ for context.  But generally the module owner/peers are like "So many reviews!  Hey, why isn't <person> doing reviews?  They should do reviews!  Welcome to being a peer, <person>!" :)  I think I'm probably going to only be doing reviews where an existing peer defers the review to me for a while longer yet as I continue to learn all the dark magic and idioms.
This patch also changes the order in which pending openWindow operations
are executed.
Attachment #8809503 - Attachment is obsolete: true
Attachment #8810946 - Flags: review?(bkelly)
Comment on attachment 8810946 [details] [diff] [review]
Use strong refs for pending openWindow operations.

Review of attachment 8810946 [details] [diff] [review]:
-----------------------------------------------------------------

I think it would be preferable if we just make OpenWindowRunnable an nsIObserver and have it register itself as the observer in this case.  That way we don't have to go through ServiceWorkerPrivate at all.
Attachment #8810946 - Flags: review?(bkelly)
Attachment #8810947 - Flags: review?(bkelly)
Comment on attachment 8829875 [details] [diff] [review]
Use OpenWindowRunnable as an observer when waiting for fennec to start before executing an open window operation.

Review of attachment 8829875 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/workers/ServiceWorkerClients.cpp
@@ +598,3 @@
>        nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>        NS_ENSURE_STATE(os);
> +      os->AddObserver(this, "BrowserChrome:Ready", true);

AddObserver() is fallible:

https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsObserverService.cpp#222

Please check the return code.  You should probably do the StoreISupports() only after you know this is successful.

nit: Please add a /* weak ref */ comment here for the bare boolean.

@@ +629,5 @@
> +    WorkerPrivate* workerPrivate = mPromiseProxy->GetWorkerPrivate();
> +    MOZ_ASSERT(workerPrivate);
> +
> +    nsCOMPtr<nsIPrincipal> principal = workerPrivate->GetPrincipal();
> +    MOZ_ASSERT(principal);

Can you make these MOZ_DIAGNOSTIC_ASSERT()?  I think we should use diagnostic more for cheap checks like this.

::: dom/workers/ServiceWorkerPrivate.h
@@ +61,5 @@
>  // Adding an API function for a new event requires calling |SpawnWorkerIfNeeded|
>  // with an appropriate reason before any runnable is dispatched to the worker.
>  // If the event is extendable then the runnable should inherit
>  // ExtendableEventWorkerRunnable.
> +class ServiceWorkerPrivate final : public nsISupports

Does this really need to stay nsISupports?  Can't we just implement a cycle collecting AddRef/Release instead?
Attachment #8829875 - Flags: review?(bkelly) → review+
Addressed comments. SWP no longer inherits nsISupports.
Attachment #8810946 - Attachment is obsolete: true
Attachment #8810947 - Attachment is obsolete: true
Attachment #8829875 - Attachment is obsolete: true
I had to drop cycle collection from OpenWindowRunnable because it inherits
mozilla::Runnable which has thread safe ref counting and we would end up with
two mRefCnt members, which is not allowed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=36f00cf77fa28bc7bd357bd8ebdba127e728c82d&selectedJob=72309990
Attachment #8830285 - Attachment is obsolete: true
Attached patch diff.patchSplinter Review
Attachment #8831998 - Flags: review?(bkelly)
Attachment #8831998 - Flags: review?(bkelly) → review+
Pushed by catalin.badea392@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60b3df69c5a9
Use OpenWindowRunnable as an observer when waiting for fennec to start before executing an open window operation. r=bkelly
https://hg.mozilla.org/mozilla-central/rev/60b3df69c5a9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: