Handle offline cache updates in parent process after windows are being torn down in child processes

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

Attachments

(1 attachment)

Posted patch changes.patchSplinter Review
We're currently sending offline cache update notifications to child processes even after we've started the PBrowser teardown sequence, leading to MsgRouteErrors. Attached patch should fix the issue.

I also removed an unnecessary function (Kill) and a nonexistent function (RefcountHitZero), and added a few more MOZ_OVERRIDE annotations.
Attachment #8360131 - Flags: review?(honzab.moz)
Comment on attachment 8360131 [details] [diff] [review]
changes.patch

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

untested locally.  please push to try (and expose the link here) before landing.

r=honzab

::: dom/ipc/TabParent.cpp
@@ +280,5 @@
> +  const InfallibleTArray<POfflineCacheUpdateParent*>& ocuParents =
> +    ManagedPOfflineCacheUpdateParent();
> +  for (uint32_t i = 0; i < ocuParents.Length(); ++i) {
> +    static_cast<mozilla::docshell::OfflineCacheUpdateParent*>(ocuParents[i])->
> +      StopSendingMessagesToChild();

please, to a local var first (for debugging)

@@ +1578,4 @@
>  {
> +  // A reference is guaranteed here, no need for a nsRefPtr.
> +  auto update =
> +    static_cast<mozilla::docshell::OfflineCacheUpdateParent*>(aActor);

I'd still rather see it used here.

::: uriloader/prefetch/OfflineCacheUpdateParent.cpp
@@ -60,5 @@
> -    // Make sure the service has been initialized
> -    nsOfflineCacheUpdateService* service =
> -        nsOfflineCacheUpdateService::EnsureService();
> -    if (!service)
> -        return;

Please leave this here, it inits the NSPR log.
Attachment #8360131 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #1)
> > -    // Make sure the service has been initialized
> > -    nsOfflineCacheUpdateService* service =
> > -        nsOfflineCacheUpdateService::EnsureService();
> > -    if (!service)
> > -        return;
> 
> Please leave this here, it inits the NSPR log.

Hrm, can I remove the null check then? Returning early from a constructor is... weird. Luckily failure is handled in Schedule though.
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #2)
> Hrm, can I remove the null check then? Returning early from a constructor
> is... weird. Luckily failure is handled in Schedule though.

Definitely, go ahead.
Comment on attachment 8360131 [details] [diff] [review]
changes.patch

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

::: dom/ipc/TabParent.cpp
@@ +1578,3 @@
>  {
> +  // A reference is guaranteed here, no need for a nsRefPtr.
> +  auto update =

btw, are you sure auto is supported by all compilers we claim to build on?
(In reply to Honza Bambas (:mayhemer) from comment #5)
> btw, are you sure auto is supported by all compilers we claim to build on?

Yep!
blocking a blocker
blocking-b2g: --- → 1.3+
https://hg.mozilla.org/mozilla-central/rev/c225b6041c38
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.