Closed Bug 811315 Opened 12 years ago Closed 12 years ago

Move everything.me wrapper out of the home screen process

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: timdream, Assigned: cjones)

References

Details

Attachments

(7 files)

1.34 KB, patch
benfrancis
: review+
Details | Diff | Splinter Review
2.00 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
18.60 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
2.66 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
3.70 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
2.27 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
27.36 KB, patch
Details | Diff | Splinter Review
This is what we discussed in pull request of bug 808227. The wrapper launched websites in a window.open() frame opened by home screen app, and stay in the home screen process.

Triage: should be blocking+ because if user install no 3rd party apps from Marketplaces but instead always use websites listed in everything.me or added bookmark, the home screen app will host too many frame and our memory management strategy will fail.
From inspecting window_manager, I found that the way we handle wrapper frame looks like a local protocol. The 3rd parameter is a JSON string.

I think we can make most out of it by dropping the frameElement received from the element, and create another <iframe mozbrowser remote> to host the given url.

The only thing I would worry is that this iframe will be using datajar(?) of the system app. Can we set mozapp=app://browser.gaiamobile.org/ in this case? Does that make wrappers sharing cookies with the browser app?
Tim, on Friday there was a discussion including Chris Jones, Jonas, Vivien and Fabrice where some proposals were made about how to go about this.

They can explain better than I can, but there may be a way to get these frames to run in a separate process to the homescreen and share a data jar separate from the system app. It would require platform changes though.
Summary: Move wrapper out of the home screen process → Move everything.me wrapper out of the home screen process
> The only thing I would worry is that this iframe will be using datajar(?) of the system app.

It would use the datajar of browsers within the system app, which is distinct from non-browsers within the system app.

> Can we set mozapp=app://browser.gaiamobile.org/ in this case? Does that make wrappers sharing 
> cookies with the browser app?

It would, which is probably not what you want.
(In reply to Ben Francis [:benfrancis] from comment #2)
> Tim, on Friday there was a discussion including Chris Jones, Jonas, Vivien
> and Fabrice where some proposals were made about how to go about this.
> 

Well, looks like I'd missed it... no worries, just want to make sure we don't drop this -- I don't want to restart my phone every time I scroll through Facebook!

(In reply to Justin Lebar [:jlebar] from comment #3)
> > Can we set mozapp=app://browser.gaiamobile.org/ in this case? Does that make wrappers sharing 
> > cookies with the browser app?
> 
> It would, which is probably not what you want.

I would personally like the wrapper frame sharing cookies with the browser app, but I guess from previously decisions we are not doing that (as a requirement for wrapper). Forget what I asked here then.
> I would personally like the wrapper frame sharing cookies with the browser app

No -- if you did <iframe mozapp=app://browser.gaiamobile.org>, everything.me would share cookies with the /browser app/, not with browser content.

So the everything.me content would be able to read your browsing history, but would not able to auto-log-in to websites you're already logged in to.
I definitely don't think that we should be using the data-jar of any *app*. Each app has two data jars, one for app content and one for browser content. We should be using the browser-content data jar of some app, but ideally not the system app since that is the same data jar as we use for payment providers which makes it easier to launch CSRF and clickjacking attacks against it.

The solution we discussed last week was to install a hidden dummy app and use its browser-content data-jar. I filed bug 811344 on adding the ability to the platform of opening an <iframe mozbrowser> which uses the browser-content data-jar of a specified app.

Once we have that fixed, gaia can use it to open new out-of-process <iframe mozbrowser>s which point to the datajar that we want to use.

An even better solution would be if we could use a separate data jar for each homescreen bookmark. This would make homescreen bookmarks behave even more like apps. Unfortunately our app registry doesn't really have the support for that though, so I think we'll have to live without that for now.
Depends on: 811344
(In reply to Jonas Sicking (:sicking) from comment #6)
> The solution we discussed last week was to install a hidden dummy app and
> use its browser-content data-jar. I filed bug 811344 on adding the ability
> to the platform of opening an <iframe mozbrowser> which uses the
> browser-content data-jar of a specified app.

Question: If we cannot create independent datajar for each bookmarks, can the shared datajar (the browser-content data-jar of a specified app) be the one of the Browser app? I found it confusing if I am required to keep two Facebook sessions on the same phone. Besides, the hidden app who keeps the datajar would have to unhidden in the Settings app so I can clear private data.

Having a hidden dummy app sounds like a being in a valley of two peaks -- we are in the right direction but we shouldn't stay there, for v1.
> can the shared datajar (the browser-content data-jar of a specified app) be the one of the Browser 
> app?

Sure; you can do this right now without any platform work, I believe.  Just create a mozbrowser inside a mozapp whose manifest is for the browser.
(In reply to Justin Lebar [:jlebar] from comment #8)
> > can the shared datajar (the browser-content data-jar of a specified app) be the one of the Browser 
> > app?
> 
> Sure; you can do this right now without any platform work, I believe.  Just
> create a mozbrowser inside a mozapp whose manifest is for the browser.

Nested mozbrowser iframes will require Gaia to re-implement a single tab browser within the first level, handling modaldialgs/http auth/reload/UIs etc. Fixing bug 811344 sounds easier than all that.
> Fixing bug 811344 sounds easier than all that.

I'd be happy to review a patch, if you're volunteering.  :)

I explained in that bug a number of reasons why I don't think we should do it at this point.  I also gave some possible workarounds.
(In reply to Justin Lebar [:jlebar] from comment #10)
> > Fixing bug 811344 sounds easier than all that.
> 
> I'd be happy to review a patch, if you're volunteering.  :)
> 
> I explained in that bug a number of reasons why I don't think we should do
> it at this point.  I also gave some possible workarounds.

Understood. That's a long, long discussion :-/
Okay, so what's actionable here?  We can't + this bug and have it sit there.  Can someone summarize what needs to get done explicitly and who would be the person to take this up before we add this to our list of "to do"?  Thanks!
See the discussion in the dependent bug.
Flags: needinfo?(dscravaglieri)
(In reply to Faramarz (:faramarz) from comment #13)
> Okay, so what's actionable here?  We can't + this bug and have it sit there.
> Can someone summarize what needs to get done explicitly and who would be the
> person to take this up before we add this to our list of "to do"?  Thanks!

We will try to implement justin's solution from https://bugzilla.mozilla.org/show_bug.cgi?id=811344#c4 .

This require some changes in Gaia in the way the system app open bookmarks from the homescreen. Alexandre Poirot will be working on it, we discussed it this afternoon about how it should work. I expect a result in the next few days.
Let me make sure we're on the same page about goals.  There are two cases here

 1. User loads apps from within the e.me UI itself.  The UX for this is analogous to having a single "browser tab" open: each subsequently loaded app replaces the previous in a single "app frame".  We want to run that app frame in its own process.  The proposal in bug 811344 comment 4 should work fine for that, with some hackery dackery doo.

 2. User loads "link app" created by bookmarking page in browser or "installing" e.me app.  The UX for this is like launching any other "actually installed" app.  We want each of these "link apps" to run in their own processes.  The proposal in bug 811344 comment 4 doesn't handle that.  However, with the right window.open() hackery we could probably make something analogous to the current homescreen, but with all the e.me apps and bookmarks in their own process.

Getting to process-per-link-app in (2) is the hard part.  I think there've been three solutions proposed for that

 A. Support dynamically-created app manifests, so that the "link apps" can be installed just like regular apps.  This is the solution I prefer.

 B. Add support for something like window.open(..., "...remote=force...") to allow special content to create out-of-process popups.  Because this would be an opt-in attribute for special content, there's no compat worries.  We would return null from the window.open() call.

 C. Add support for something like directly creating <iframe mozapp="foo" mozbrowser="true" remote="force"> each in its own process.  jlebar points out that this is hard.

sicking/fabrice, what would be required to implement (A)?

jlebar, you said you explained why (B) is hard, but I'm not following.  Let me try to go through your older comments and see what I'm missing.
> jlebar, you said you explained why (B) is hard, but I'm not following.

(B) is complicated for exactly the same reason (C) is complicated.

We currently decide which process a frame goes into based on the attributes on the frame element.  With (B) and (C), the attributes on the frame element are no longer sufficient to specify which process the frame goes into, because you can have multiple frame elements with exactly the same app and browser attributes which go into different processes.

This is important for the case when the frame element calls window.open (I believe).  It's also important for general sanity.
(In reply to Justin Lebar [:jlebar] from comment #17)
> > jlebar, you said you explained why (B) is hard, but I'm not following.
> 
> (B) is complicated for exactly the same reason (C) is complicated.
> 

I think I may finally understand.  Is the problem with (B) that (i) we'd end up opening an <iframe mozapp> that's the sibling of the opening <iframe mozapp>, which would confuse gecko; (ii) and trying to open an <iframe mozbrowser> that's the sibling of of the opening <iframe mozapp> would break the invariant that mozbrowser must nest inside its mozapp?

If so, I have a solution.
The problem with (B) (in my head -- I haven't tried to code this) has nothing to do with window.open.  It has everything to do with whether the attributes on the frame element identify which process the frame element lives in.

> Is the problem with (B) that (i) we'd end up opening an <iframe mozapp> that's the 
> sibling of the opening <iframe mozapp>, which would confuse gecko

This is fine, afaict.

> (ii) and trying to open an <iframe mozbrowser> that's the sibling of of the opening 
> <iframe mozapp> would break the invariant that mozbrowser must nest inside its mozapp?

The mozbrowser is going to live inside the system app, so I don't see how we're breaking that invariant.

Like you said on IRC, maybe it's not a big deal that the attributes on the frame element won't identify which process the frame element should live in.  If so, either (B) or (C) would be relatively straightforward.  Still a few days of work to get tests and so on.
> The mozbrowser is going to live inside the system app, so I don't see how we're breaking 
> that invariant.

Oh, you said /its/ mozapp.  Yes, this is an invariant we'd have to look closely at.
OK, well instead of dragging this out, let me propose my solution

 1. Start with plan in bug 811344 comment 4.  I don't know how large of a gaia patch this is.
 2. Set "dom.ipc.processCount" to "unlimited".  This is a 1-line gecko patch.  This will affect the browser app too, but I can live with that.  (Process-per-tab firefox!)
 3. Add a iframe attribute mozasyncpanzoom and use that instead of isBrowser to decide which rendering behavior we use.  This is probably a 20-30 line gecko patch.
 4. Use mozasyncpanzoom for browser-tab content in gaia, but don't use it for "link apps".  This is a 1-line gaia patch.
Assignee: nobody → jones.chris.g
This patch can land at any time.

The two changes mentioned in the description should be pretty self-explanatory.  The additions of the |// "| lines are to make emacs happy; java-mode doesn't know about literal regexps so most of browser.js renders like string content.
I'm still not convinced this is the right thing to do, but
 - running "link apps" out of process is more important IMHO
 - I doubt many users open multiple tabs anyway
 - jlebar was in favor of this approach ;)
Attachment #683463 - Flags: review?(justin.lebar+bug)
Delightfully simple.  Thanks for the refactoring here!
Attachment #683464 - Flags: review?(justin.lebar+bug)
Fat-fingered the Enter, but I can't request sr here anyway ... o_O
Component: Gaia::System → DOM
Product: Boot2Gecko → Core
Comment on attachment 683465 [details] [diff] [review]
part 3: Add a mozasyncpanzoom attribute for iframes and honor it when constructing a remote frame

Should be pretty straightforward.  Example usage is in part 0.
Attachment #683465 - Flags: superreview?(roc)
Attachment #683465 - Flags: superreview?(roc) → superreview+
Attachment #683462 - Flags: review?(ben) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
>  1. Start with plan in bug 811344 comment 4.  I don't know how large of a
> gaia patch this is.

Me, looking into gaia patch. I already have a patch in good shape, still some issues with cardview/process-manager.
I needed to move this out of the b2g component to be able to request sr, but now that we're out of there I can't request approval-gaia.

a? for gaia-master for part 0.  For now, the patch fixes an existing bug and adds a no-op attribute set.
Flags: needinfo?(21)
Comment on attachment 683462 [details] [diff] [review]
part 0: Fix setVisible() bug in the browser app and prepare it for explicit mozasyncpanzoom-ness

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

a=me.
Flags: needinfo?(21)
Comment on attachment 683463 [details] [diff] [review]
part 1: Make "browser" process limit infinite and unload the processes when all tabs are closed.  Firefox with process-per-tab!

Ugh, this will seriously regress our ability to open multiple tabs.  (And opening just a few tabs is already causing OOMs in QA's testing.)

> I'm still not convinced this is the right thing to do

Me either.  You already did the hard work wrt async pan/zoom; I'll see if I can spin a patch with an alternative approach which doesn't regress the browser.
(In reply to Justin Lebar [:jlebar] from comment #32)
> Comment on attachment 683463 [details] [diff] [review]
> part 1: Make "browser" process limit infinite and unload the processes when
> all tabs are closed.  Firefox with process-per-tab!
> 
> Ugh, this will seriously regress our ability to open multiple tabs.  (And
> opening just a few tabs is already causing OOMs in QA's testing.)
> 

Actually, it should *improve* that.  Old tabs will die in the background when new ones are opened.  At any rate, QA is probably testing without bug 798491, so I distrust early results from there.

My objection to process-per-tab is that it has the side effect of prioritizing OS processes that are in the service of the browser process over all other apps.  That is, it allows the browser to "launder" its memory usage among a set of tab processes.  I wouldn't expect a regression in loading of tabs themselves.

> > I'm still not convinced this is the right thing to do
> 
> Me either.  You already did the hard work wrt async pan/zoom; I'll see if I
> can spin a patch with an alternative approach which doesn't regress the
> browser.

Without a workload to test, I don't see how we can decide whether something regresses or not.  (And yes, I'm working on that as time allows.)  Let's start there ;).
(Not sure what info is needed from dscravag.)
Flags: needinfo?(dscravaglieri)
> Ugh, this will seriously regress our ability to open multiple tabs.  (And
> opening just a few tabs is already causing OOMs in QA's testing.)
> 
> 
> Actually, it should *improve* that.

Sorry, when I say "open multiple tabs", I mean "open multiple tabs and keep them open."  That is, the number of concurrently-open tabs we could have would decrease.  This should be true for virtually any workload, right?

You're right that this would make it less likely that opening a heavyweight page in the foreground will OOM the foreground tab (instead, it should kill the bg tabs).  It's not clear to me that's a win overall, but it might be...
Yes, we would be paying a higher per-tab overhead.

However, the UX in the case of one-process-for-all-tabs is that trying to open a new tab when memory is too low just fails.  Frowny face.  With process-per-tab, it would always succeed.

I'm not convinced that multi-tab browsing is a particularly important use case for v1, but opening multiple "link apps" sure is.  I'd rather not hang up a less-well-understood use case.
> However, the UX in the case of one-process-for-all-tabs is that trying to open a new tab 
> when memory is too low just fails.  Frowny face.  With process-per-tab, it would always 
> succeed.

Fair enough!  Let's try this approach and see if it works.
Attachment #683463 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 683464 [details] [diff] [review]
part 2: Pass scrolling-behavior hint down into TabParent instead of inferring it from app/browser

This looks great.

>diff --git a/dom/ipc/TabContext.h b/dom/ipc/TabContext.h
>+  /**
>+   * True iff the embedder requested async pan/zoom behavior for this
>+   * frame.
>+   */
>+  bool GetScrollingBehavior() const { return mScrollingBehavior; }

If this is to return bool, it seems like the name is wrong and we should have
an explicit == ASYNC_PAN_ZOOM here.  Otherwise the return type and comment
should change.

(Returning ScrollingBehavior instead of bool seems most sane here to me.)

Also, would you mind not using "Get" for this method name, since it can't
return null?
Attachment #683464 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 683465 [details] [diff] [review]
part 3: Add a mozasyncpanzoom attribute for iframes and honor it when constructing a remote frame

FWIW, remote and mozbrowser are checked with HasAttr, so perhaps you want to do that here, too.  Either way is fine with me.
Attachment #683465 - Flags: review?(justin.lebar+bug) → review+
(In reply to Justin Lebar [:jlebar] from comment #38)
> Comment on attachment 683464 [details] [diff] [review]
> part 2: Pass scrolling-behavior hint down into TabParent instead of
> inferring it from app/browser
> 
> This looks great.
> 
> >diff --git a/dom/ipc/TabContext.h b/dom/ipc/TabContext.h
> >+  /**
> >+   * True iff the embedder requested async pan/zoom behavior for this
> >+   * frame.
> >+   */
> >+  bool GetScrollingBehavior() const { return mScrollingBehavior; }
> 
> If this is to return bool, it seems like the name is wrong and we should have
> an explicit == ASYNC_PAN_ZOOM here.  Otherwise the return type and comment
> should change.
> 
> (Returning ScrollingBehavior instead of bool seems most sane here to me.)
> 

Oh haysoos, this code is entirely wrong.  It was originally a bool but then I wanted to change it to return ScrollingBehavior, which is what all the consuming code expects.  And of course the enum values happened to work out just right for the boolean return :(.  Thanks C++!

> Also, would you mind not using "Get" for this method name, since it can't
> return null?

I recall C++ being unhappy with the type ScrollingBehavior and the method ScrollingBehavior() defined in the same scope, but if I misremember I'll change that.
(In reply to Justin Lebar [:jlebar] from comment #39)
> Comment on attachment 683465 [details] [diff] [review]
> part 3: Add a mozasyncpanzoom attribute for iframes and honor it when
> constructing a remote frame
> 
> FWIW, remote and mozbrowser are checked with HasAttr, so perhaps you want to
> do that here, too.  Either way is fine with me.

I was keying off of http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1491 , which checks for remote="true" exactly IIUC.
> which checks for remote="true" exactly IIUC.

Ah, you're right.  I was looking just above at the HasAttr call, but that's for something different.

mozbrowser is definitely a HasAttr check.  Either way; not a big deal.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #40)
> (In reply to Justin Lebar [:jlebar] from comment #38)
> > Comment on attachment 683464 [details] [diff] [review]
> > Also, would you mind not using "Get" for this method name, since it can't
> > return null?
> 
> I recall C++ being unhappy with the type ScrollingBehavior and the method
> ScrollingBehavior() defined in the same scope, but if I misremember I'll
> change that.

C++ indeed craps its pants if I try to do that.  Sorry.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> Created attachment 683463 [details] [diff] [review]
> part 1: Make "browser" process limit infinite and unload the processes when
> all tabs are closed.  Firefox with process-per-tab!
> 

Cute; this patch caused a bunch of M2 timeouts and crashes, apparently because unloading the processes triggered a bunch of existing bugs.  Yay, I guess.
The problem here was that we can get a sequence of events like the following, where each bullet is an event-loop iteration
 - ContentParent::NotifyTabDestroyed()
   * enqueue task to run ContentParent::ShutDownProcess() to "really" shut down
 - ...
 - ContentParent::GetNewOrUsed() is called
   * creates doomed TabParent
 - ...
 - ContentParent::ShutDownProcess()
   * the doomed TabParent dies here

So in the browser-element tests, the symptom was random disappearing <iframe remote=true>, with predictable bad consequences.

It's not possible to hit this with current b2g or FF builds, but it would be possible to hit this in both FF (obviously) *and* b2g configurations in the field, after these patches.  So yay tests! :)
Attachment #685497 - Flags: review?(justin.lebar+bug)
Hopefully pretty self-explanatory: on the second try run, we got lucky and the FirstIdle task was still in the queue after ContentChild::ActorDestroy().  So cancel the task if it's still around when we hit ActorDestroy() in debug builds.
Attachment #685510 - Flags: review?(justin.lebar+bug)
So I tried to hack Gaia in order to use two frames, like this:
  <iframe mozapp mozbrowser>
    <iframe mozbrowser remote>
  </iframe>
These frames are hosted by the system app.
And I can't find any hack (tried various one) that fullfill all requirements.
* if I set mozapp to browser manifest (allows to share cookies with browser app), or a dedicated fake app, I hit cross domain restrictions. So that we would have to pipe various calls and events (mozbrowserdialog, mozlocationchange, canGoBack and so on) between the mozapp iframe and the system app. It would be quite error prone and it isn't really easy to communicate between those frames because of cross domain limitations...
* if I set mozapp to the system app, there isn't such complications. Cookies and data aren't shared with the system app itself, but it is shared with content frames used in system app. The main issue here is that it will share data with navigator.mozPay and browser ID frame.

Given that, I'll build a patch with the second approach, so that e.me apps won't crash homescreen anymore. Then I'd happily iterate on this and use new platform feature (cross domain, data jar attribute).
(Suggestion of workaround are still welcomed)
basecamp + because is blocking bug 807438 which is a blocker for C2 Milestone
blocking-basecamp: ? → +
Priority: -- → P2
(In reply to Alexandre Poirot (:ochameau) from comment #50)
> * if I set mozapp to browser manifest (allows to share cookies with browser
> app), or a dedicated fake app, I hit cross domain restrictions.

Which cross-domain restrictions are these?

> So that we
> would have to pipe various calls and events (mozbrowserdialog,
> mozlocationchange, canGoBack and so on) between the mozapp iframe and the
> system app. It would be quite error prone and it isn't really easy to
> communicate between those frames because of cross domain limitations...

Yeah, wrapping the "mozbrowser" API is the sucky part of this approach.

> * if I set mozapp to the system app, there isn't such complications. Cookies
> and data aren't shared with the system app itself, but it is shared with
> content frames used in system app. The main issue here is that it will share
> data with navigator.mozPay and browser ID frame.

The problem with this is that we can't clear data for "link apps" without blowing away mozPay and ID data, too.  (That's mostly a problem of the mozpay and ID implementations, but we're past the point of no return on those.)
 
> Given that, I'll build a patch with the second approach, so that e.me apps
> won't crash homescreen anymore. Then I'd happily iterate on this and use new
> platform feature (cross domain, data jar attribute).
> (Suggestion of workaround are still welcomed)

I'd to understand more about the problems you ran into when using a dedicated "link-app launcher app" to host the e.me and browser-link shortcut apps.  I don't think using the system app jar is necessarily wrong, but it has the downsides I listed above.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #52)
> Which cross-domain restrictions are these?

- We can listen to mozbrowser events from system app as we can't access to mozapp iframe `contentWindow` and listen to the mozbrowser iframe events. And mozbrowser events don't bubble-up up to the system app. That's what has been discussed here for a while now, but it appears that piping all mozbrowser API is quite error prone. Proper mozbrowsershowmodaldialog piping is, by itself, a challenging task.
- mozapp frame can't easily pipe messages to the system app, as `parent` is equal to `window` (so no `parent.postMessage()`) and events fired in the mozapp document doesn't bubble on the iframe (can't fire custom event).

> I'd to understand more about the problems you ran into when using a
> dedicated "link-app launcher app" to host the e.me and browser-link shortcut
> apps.  I don't think using the system app jar is necessarily wrong, but it
> has the downsides I listed above.

* has to pipe everything (I thought that mozbrowser event would have bubbled up, so that we would just have to pipe methods like goBack, canGoBack,...)
* that would introduce a significant regression risk
* dialog implementation between system app and browser app will diverge significantly
* I don't know yet how to pipe messages between system and the mozapp frame document

Using system app data jar isn't an ideal solution. It is meant to be a temporary fix that stop e.me crashing issues, until we figure out how to tweak the platform to get a proper support.
I guess it's worth re-raising the question of it's better to fix this by fixing bug 811344. It's definitely more work on the platform side, but it might be less overall work.
> (I thought that mozbrowser event would have bubbled up, so that we would just have to 
> pipe methods like goBack, canGoBack,...)

That would be hard to implement in Gecko for the same reasons you cite.
Comment on attachment 685497 [details] [diff] [review]
part 0.9: Fix pre-existing race condition that allows dying ContentParents to accidentally try to load new TabParents

> void
> ContentParent::ShutDownProcess()
> {
>-  if (mIsAlive) {
>+  if (!mIsDestroyed) {
>     const InfallibleTArray<PIndexedDBParent*>& idbParents =
>       ManagedPIndexedDBParent();
>     for (uint32_t i = 0; i < idbParents.Length(); ++i) {
>       static_cast<IndexedDBParent*>(idbParents[i])->Disconnect();
>     }
> 
>     // Close() can only be called once.  It kicks off the
>     // destruction sequence.
>     Close();
>+    mIsDestroyed = true;

Do you need to set mIsDestroyed after the other call to close() (on xpcom-shutdown)?
Attachment #685497 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 685510 [details] [diff] [review]
part 0.91: Fix another pre-existing race condition where FirstIdle might run after ContentChild::ActorDestroy()

>+static CancelableTask* sFirstIdleTask;
>+
> static void FirstIdle(void)
> {
>+    sFirstIdleTask = nullptr;

Would you mind adding a MOZ_ASSERT(sFirstIdleTask)?  That would make the intent
here clearer to me.
Attachment #685510 - Flags: review?(justin.lebar+bug) → review+
(In reply to Justin Lebar [:jlebar] from comment #56)
> Comment on attachment 685497 [details] [diff] [review]
> part 0.9: Fix pre-existing race condition that allows dying ContentParents
> to accidentally try to load new TabParents
> 
> > void
> > ContentParent::ShutDownProcess()
> > {
> >-  if (mIsAlive) {
> >+  if (!mIsDestroyed) {
> >     const InfallibleTArray<PIndexedDBParent*>& idbParents =
> >       ManagedPIndexedDBParent();
> >     for (uint32_t i = 0; i < idbParents.Length(); ++i) {
> >       static_cast<IndexedDBParent*>(idbParents[i])->Disconnect();
> >     }
> > 
> >     // Close() can only be called once.  It kicks off the
> >     // destruction sequence.
> >     Close();
> >+    mIsDestroyed = true;
> 
> Do you need to set mIsDestroyed after the other call to close() (on
> xpcom-shutdown)?

Oh, ew.  We should be using ShutDownProcess() there.  Thanks for the catch.
(In reply to Justin Lebar [:jlebar] from comment #57)
> Comment on attachment 685510 [details] [diff] [review]
> part 0.91: Fix another pre-existing race condition where FirstIdle might run
> after ContentChild::ActorDestroy()
> 
> >+static CancelableTask* sFirstIdleTask;
> >+
> > static void FirstIdle(void)
> > {
> >+    sFirstIdleTask = nullptr;
> 
> Would you mind adding a MOZ_ASSERT(sFirstIdleTask)?  That would make the
> intent
> here clearer to me.

Done.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8aa207f87eb6

Needs to bake a day or so to catch unexpected browser-tab regressions.  Not too useful without the app launching fixes anyway.
https://hg.mozilla.org/mozilla-central/rev/8aa207f87eb6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(In reply to Alexandre Poirot (:ochameau) from comment #28)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> >  1. Start with plan in bug 811344 comment 4.  I don't know how large of a
> > gaia patch this is.
> 
> Me, looking into gaia patch. I already have a patch in good shape, still
> some issues with cardview/process-manager.

Is there a bug on file for this?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #64)
> (In reply to Alexandre Poirot (:ochameau) from comment #28)
> > Me, looking into gaia patch. I already have a patch in good shape, still
> > some issues with cardview/process-manager.
> 
> Is there a bug on file for this?

That was originaly this bug, but it changed from component.
If you don't see any problem in that, I'm planning to post that to bug 807438 instead.
(In reply to Alexandre Poirot (:ochameau) from comment #65)
> That was originaly this bug, but it changed from component.
> If you don't see any problem in that, I'm planning to post that to bug
> 807438 instead.

If that's the case, can someone update the title to more accurately describe what is being done in this bug? Thanks!

/me almost thought these two issues are duplicates
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: