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)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: timdream, Assigned: cjones)
References
Details
Attachments
(7 files)
(deleted),
patch
|
benfrancis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Reporter | ||
Comment 1•12 years ago
|
||
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?
Comment 2•12 years ago
|
||
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.
Updated•12 years ago
|
Summary: Move wrapper out of the home screen process → Move everything.me wrapper out of the home screen process
Comment 3•12 years ago
|
||
> 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.
Reporter | ||
Comment 4•12 years ago
|
||
(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.
Comment 5•12 years ago
|
||
> 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
Reporter | ||
Comment 7•12 years ago
|
||
(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.
Comment 8•12 years ago
|
||
> 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.
Reporter | ||
Comment 9•12 years ago
|
||
(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.
Comment 10•12 years ago
|
||
> 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.
Reporter | ||
Comment 11•12 years ago
|
||
(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 :-/
Comment 13•12 years ago
|
||
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!
Comment 14•12 years ago
|
||
See the discussion in the dependent bug.
Updated•12 years ago
|
Flags: needinfo?(dscravaglieri)
Comment 15•12 years ago
|
||
(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.
Assignee | ||
Comment 16•12 years ago
|
||
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.
Comment 17•12 years ago
|
||
> 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.
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Comment 19•12 years ago
|
||
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.
Comment 20•12 years ago
|
||
> 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.
Assignee | ||
Comment 21•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: nobody → jones.chris.g
Assignee | ||
Comment 22•12 years ago
|
||
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.
Assignee | ||
Comment 23•12 years ago
|
||
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)
Assignee | ||
Comment 24•12 years ago
|
||
Delightfully simple. Thanks for the refactoring here!
Attachment #683464 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #683465 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 26•12 years ago
|
||
Fat-fingered the Enter, but I can't request sr here anyway ... o_O
Component: Gaia::System → DOM
Product: Boot2Gecko → Core
Assignee | ||
Comment 27•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #683462 -
Flags: review?(ben)
Attachment #683465 -
Flags: superreview?(roc) → superreview+
Updated•12 years ago
|
Attachment #683462 -
Flags: review?(ben) → review+
Comment 28•12 years ago
|
||
(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.
Blocks: 807438
Assignee | ||
Comment 29•12 years ago
|
||
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 30•12 years ago
|
||
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)
Assignee | ||
Comment 31•12 years ago
|
||
Comment 32•12 years ago
|
||
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.
Assignee | ||
Comment 33•12 years ago
|
||
(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 ;).
Assignee | ||
Comment 34•12 years ago
|
||
(Not sure what info is needed from dscravag.)
Flags: needinfo?(dscravaglieri)
Comment 35•12 years ago
|
||
> 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...
Assignee | ||
Comment 36•12 years ago
|
||
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.
Comment 37•12 years ago
|
||
> 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.
Updated•12 years ago
|
Attachment #683463 -
Flags: review?(justin.lebar+bug) → review+
Comment 38•12 years ago
|
||
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 39•12 years ago
|
||
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+
Assignee | ||
Comment 40•12 years ago
|
||
(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.
Assignee | ||
Comment 41•12 years ago
|
||
(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.
Comment 42•12 years ago
|
||
> 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.
Assignee | ||
Comment 43•12 years ago
|
||
(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.
Assignee | ||
Comment 44•12 years ago
|
||
Assignee | ||
Comment 45•12 years ago
|
||
(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.
Assignee | ||
Comment 46•12 years ago
|
||
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)
Assignee | ||
Comment 47•12 years ago
|
||
Assignee | ||
Comment 48•12 years ago
|
||
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)
Assignee | ||
Comment 49•12 years ago
|
||
Comment 50•12 years ago
|
||
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)
Comment 51•12 years ago
|
||
basecamp + because is blocking bug 807438 which is a blocker for C2 Milestone
blocking-basecamp: ? → +
Priority: -- → P2
Assignee | ||
Comment 52•12 years ago
|
||
(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.
Comment 53•12 years ago
|
||
(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.
Comment 55•12 years ago
|
||
> (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 56•12 years ago
|
||
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 57•12 years ago
|
||
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+
Assignee | ||
Comment 58•12 years ago
|
||
(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.
Assignee | ||
Comment 59•12 years ago
|
||
(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.
Assignee | ||
Comment 60•12 years ago
|
||
Assignee | ||
Comment 61•12 years ago
|
||
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.
Comment 62•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 63•12 years ago
|
||
Assignee | ||
Comment 64•12 years ago
|
||
(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?
Comment 65•12 years ago
|
||
(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.
Assignee | ||
Comment 66•12 years ago
|
||
wfm
Reporter | ||
Comment 67•12 years ago
|
||
(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
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•