Closed
Bug 1121905
Opened 10 years ago
Closed 10 years ago
Reorder things of remote frame loading to send child process the URL earlier
Categories
(Core :: DOM: Content Processes, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: ting, Assigned: ting)
References
Details
Attachments
(1 file, 5 obsolete files)
9.40 KB,
patch
|
Details | Diff | Splinter Review |
Currently things are happend with following order in nsFrameLoader::ReallyStartLoadinInternal() when it is a remote frame:
remote-browser-pending,
TryRemoteBrowser(),
ShowRemoteFrame() & remote-browser-shown,
LoadURL()
Would like to change it to:
TryRemoteBrowser(),
LoadURL(),
remote-browser-pending,
ShowRemoteFrame() & remote-browser-shown
so content process can receive the url earlier. Note the fd of application.zip is still needed for child process to start loading (bug 1119692), otherwise receiving the url earlier doesn't help anything.
Will need to disable |mShown| checking in TabParent::LoadURL(), and make sure not breaking any assumptions from event listeners.
Check bug 1110624 comment 14 for some more details about the events.
Assignee | ||
Comment 1•10 years ago
|
||
Move remote-browser-pending after TryRemoteBrowser() could possibly miss the hello from BrowserElementChild.js.
Assignee | ||
Comment 2•10 years ago
|
||
The |mShown| checking in TabParent::LoadURL() is from bug 714861: https://hg.mozilla.org/mozilla-central/rev/9d00516b0ad7. I read the patch, but don't get the idea why it is needed.
Justin, do you remember why?
Flags: needinfo?(justin.lebar+bug)
Comment 3•10 years ago
|
||
I don't know if he reads his bugmail. You may want to email him directly.
Assignee | ||
Comment 4•10 years ago
|
||
Thank you for the reminding! I've sent him an email offline, also NI cjones as he was the reviewer.
:cjones, do you know/remember the reason why the |mShown| checking [1] is needed? Thanks.
[1] https://hg.mozilla.org/mozilla-central/rev/9d00516b0ad7#l7.67
Flags: needinfo?(justin.lebar+bug) → needinfo?(cjones.bugs)
Comment 5•10 years ago
|
||
(mccr8 is right, I'm not reading bugmail.)
Sorry, I'm afraid I'm not going to be much help here.
It appears that something would go wrong if you called LoadURL before Show(). I vaguely remember things breaking in that case, but exactly what broke or how, I don't know. Guess I should have put a comment. :-/ The good news is that things have probably changed a lot since then, so if it works now without the check, you can probably get rid of it. I realize that's a lot of "probably"s. :)
Assignee | ||
Comment 6•10 years ago
|
||
Thanks for the info. I did a Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d5337b89d3, and there're two failed cases with Firefox:
dom/browser-element/mochitest/test_browserElement_oop_LoadEvents.html
-> Only mozbrowserloadend is received, no mozbrowserloadstart and mozbrowserlocationchange
dom/browser-element/mochitest/test_browserElement_oop_SecurityChange.htm
-> Does not receive mozbrowsersecuritychange
dom/canvas/test/webgl-mochitest/test_webgl_request_mismatch.html
-> Crash, not sure is it related to the change
STATE_START happens before loading BrowserElementChildPreload.js, I'll take a look.
Flags: needinfo?(cjones.bugs)
(In reply to Ting-Yu Chou [:ting] from comment #0)
> Currently things are happend with following order in
> nsFrameLoader::ReallyStartLoadinInternal() when it is a remote frame:
>
> remote-browser-pending,
> TryRemoteBrowser(),
> ShowRemoteFrame() & remote-browser-shown,
> LoadURL()
>
The problem was, IIRC, that we didn't always see TabParent initialization in this order. The order of those calls into TabParent depended on how the frame loader / iframe ended up being added into the DOM and "shown" (in the CSS display sense). The gaia frontend always initialized <iframe remote> in a way we supported, IIRC, but the <browser remote> (i.e. desktop XUL) tests would occasionally trigger an order that broke rendering. (The reason rendering broke, again IIRC, is that TabParent didn't have a layout frame yet and couldn't find an nsIWidget to initialize from. And there wasn't an obvious fix, partly because I didn't understand all the possible states FrameLoader could get into.)
The initialization of FrameLoader / TabParent / TabChild is quite delicate as I'm sure you know, or at least was when this code was written, so we decided to land this workaround to guard against an occasional bug we only saw in tests of dead code at the time (desktop OOP frontend). The alternative was pretty much open-ended, and bug 714861 was blocking most of the b2g project at the time. I (and probably Justin too) apologize for punting this technical debt into the future, but if I were back in the same circumstances I would probably land the patch again :/.
Feel free to ni? me if there are any other questions I can (try to) help answer about this code :). I'm sure there's a much simpler way to do this; your best path forward might be an incremental refactoring.
Assignee | ||
Comment 8•10 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be740ba8961f
Still there're failed tests.
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Chris Jones [:cjones] inactive; ni?/f?/r? if you need me from comment #7)
> answer about this code :). I'm sure there's a much simpler way to do this;
> your best path forward might be an incremental refactoring.
I believe so. Thank you for explaining me :)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → tchou
Status: NEW → ASSIGNED
QA Contact: tchou
Assignee | ||
Updated•10 years ago
|
QA Contact: tchou
Assignee | ||
Comment 10•10 years ago
|
||
Try looks better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf0b83ec917a
Attachment #8559647 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8560327 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8563281 -
Flags: review?(bugs)
Comment 12•10 years ago
|
||
Comment on attachment 8563281 [details] [diff] [review]
patch v2
> TabChild::RecvLoadURL(const nsCString& uri)
> {
>- SetProcessNameToAppName();
>+ if (!InitTabChildGlobal()) {
>+ return false;
>+ }
>
> nsresult rv = WebNavigation()->LoadURI(NS_ConvertUTF8toUTF16(uri).get(),
> nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP |
> nsIWebNavigation::LOAD_FLAGS_DISALLOW_INHERIT_OWNER,
> nullptr, nullptr, nullptr);
> if (NS_FAILED(rv)) {
> NS_WARNING("WebNavigation()->LoadURI failed. Eating exception, what else can I do?");
> }
>
>+ SetProcessNameToAppName();
>+
I don't understand this change.
Why we want to call SetProcessNameToAppName() so late?
Other than that, looks good (although rather scary).
Attachment #8563281 -
Flags: review?(bugs) → review-
Comment 13•10 years ago
|
||
Is it "Webapps:GetList" sync message from child to parent which causes problems?
If so, sounds like we should send that data explicitly from parent to child so that
child wouldn't need to ask for it.
Assignee | ||
Comment 14•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #12)
> Why we want to call SetProcessNameToAppName() so late?
Since the goal is to start loading as early as possible, and I don't see any code depend on the process name which has to be done before loading. So I move it after LoadURI().
Assignee | ||
Comment 15•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #13)
> Is it "Webapps:GetList" sync message from child to parent which causes
> problems?
> If so, sounds like we should send that data explicitly from parent to child
> so that
> child wouldn't need to ask for it.
Could be, but I don't see DOMApplicationRegistry.resetList() (which sends "Webapps:GetList") from the profiles at bug 1110624 comment 6 and comment 14. We can deal with it later if profile shows it is a problem. How do you think?
Comment 16•10 years ago
|
||
(In reply to Ting-Yu Chou [:ting] (away 2/18~2/25) from comment #15)
> Could be, but I don't see DOMApplicationRegistry.resetList() (which sends
> "Webapps:GetList") from the profiles at bug 1110624 comment 6 and comment
> 14.
What is then the thing you try to avoid with moving SetProcessNameToAppName() ?
SetProcessNameToAppName() itself is super simple method, but is it accessing
mozIApplication which makes it slow?
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #16)
> What is then the thing you try to avoid with moving
> SetProcessNameToAppName() ?
> SetProcessNameToAppName() itself is super simple method, but is it accessing
> mozIApplication which makes it slow?
I was just try to let content process to start loading earlier. I have reverted that change and rebased.
Attachment #8563281 -
Attachment is obsolete: true
Attachment #8569603 -
Flags: review?(bugs)
Comment 18•10 years ago
|
||
Comment on attachment 8569603 [details] [diff] [review]
patch v3
>+ if (!mRemoteBrowser && !TryRemoteBrowser()) {
>+ NS_WARNING("Couldn't create child process for iframe.");
>+ return NS_ERROR_FAILURE;
> }
>
>- if (mRemoteBrowserShown || ShowRemoteFrame(nsIntSize(0, 0))) {
>- // FIXME get error codes from child
>- mRemoteBrowser->LoadURL(mURIToLoad);
>- } else {
>+ // FIXME get error codes from child
>+ mRemoteBrowser->LoadURL(mURIToLoad);
>+
>+ if (!mRemoteBrowserShown && !ShowRemoteFrame(nsIntSize(0, 0))) {
> NS_WARNING("[nsFrameLoader] ReallyStartLoadingInternal tried but couldn't show remote browser.\n");
> }
So currently ShowRemoteFrame() calls EnsureMessageManager(). That really needs to happen before we start to load anything, so that
we get (pending) frame scripts executed before LoadURL. Otherwise the setup would be racy and frame script might or might not be there
when the mURIToLoad has been loaded.
Perhaps test this all with data:text/html,foo or about:blank -like urls which don't need any data from the network.
(sorry about not thinking that earlier)
Attachment #8569603 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 19•10 years ago
|
||
Do you know why does EnsureMessageManager() initialize nsFrameMessageManager's mCallback after mRemoteBrowserShown is true?
Flags: needinfo?(bugs)
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Ting-Yu Chou [:ting] from comment #19)
> Do you know why does EnsureMessageManager() initialize
> nsFrameMessageManager's mCallback after mRemoteBrowserShown is true?
It seems because InitTabChildGlobal() is done in TabChild::RecvShow(), which TabChild::RecvLoadRemoteScript() then can load scripts.
As InitTabChildGlobal() will be done in RecvLoadURL() as well, what if I remove the mRemoteBrowserShown checking from EnsureMessageManger() and place a EnsureMessageManager() call before LoadURL()?
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #18)
> Perhaps test this all with data:text/html,foo or about:blank -like urls
> which don't need any data from the network.
I'm not sure how to test it.
From Try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=937bf9b65c43) I didn't see failed cases like this, how could I check whether there're existed test cases?
Comment 22•10 years ago
|
||
(In reply to Ting-Yu Chou [:ting] from comment #20)
> As InitTabChildGlobal() will be done in RecvLoadURL() as well, what if I
> remove the mRemoteBrowserShown checking from EnsureMessageManger() and place
> a EnsureMessageManager() call before LoadURL()?
Something like that could work.
mRemoteBrowserShown check is needed, IIRC, for the case when someone accesses frameLoader.messageManager before the load
is started (so before there is any remote browser) since at that point we can't try to load the
delayed scripts (2nd parameter to loadFrameScript).
Flags: needinfo?(bugs)
Assignee | ||
Comment 23•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #22)
> mRemoteBrowserShown check is needed, IIRC, for the case when someone
> accesses frameLoader.messageManager before the load
> is started (so before there is any remote browser) since at that point we
> can't try to load the
> delayed scripts (2nd parameter to loadFrameScript).
For some unknown reasons, checking mRemoteBrowser instead of mRemoteBrowserShown does not work...
Assignee | ||
Comment 24•10 years ago
|
||
(In reply to Ting-Yu Chou [:ting] from comment #23)
> For some unknown reasons, checking mRemoteBrowser instead of
> mRemoteBrowserShown does not work...
Scratch this, it's because of some other pieces of code I added.
Assignee | ||
Comment 25•10 years ago
|
||
Call EnsureMessageManager() before LoadURL(), and check mRemoteBrowser instead of mRemoteBrowserShown in EnsureMessageManager() for determining whether remote browser is existed.
Attachment #8569603 -
Attachment is obsolete: true
Attachment #8572530 -
Flags: review?(bugs)
Comment 26•10 years ago
|
||
Comment on attachment 8572530 [details] [diff] [review]
patch v4
Ok, so ShowRemoteFrame ends up then initializing BrowserAPI, good.
This is still super scary though, so be prepared for regressions.
Please test on device and e10s before landing.
> TabChild::RecvLoadURL(const nsCString& aURI,
> const BrowserConfiguration& aConfiguration)
> {
> SetProcessNameToAppName();
>
>+ if (!InitTabChildGlobal()) {
>+ return false;
>+ }
You should call InitTabChildGlobal before SetProcessNameToAppName
Attachment #8572530 -
Flags: review?(bugs) → review+
Comment 27•10 years ago
|
||
Comment on attachment 8572530 [details] [diff] [review]
patch v4
Uh, looks like browser element API needs to be initialized before we start loading a page. Otherwise we might get Webapps:RegisterBEP wrong time.
Attachment #8572530 -
Flags: review+ → review-
Assignee | ||
Comment 28•10 years ago
|
||
There will be 3 places calling InitializeBrowserAPI() if I add it before LoadURL(), I don't very much like that. I'd like to decouple it from frame shown...
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #27)
> Uh, looks like browser element API needs to be initialized before we start
> loading a page. Otherwise we might get Webapps:RegisterBEP wrong time.
It seems Webapps:RegisterBEP is sent when BrowserElementParent.js runs in child, does it matter to initialize browser API before sending URL in parent? Won't the message only be handled at next event loop which is after initializing the API?
Flags: needinfo?(bugs)
Comment 30•10 years ago
|
||
But we need to send the stuff related to message manager and browserAPI before starting to load
an URL.
Flags: needinfo?(bugs)
Assignee | ||
Comment 31•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #30)
> But we need to send the stuff related to message manager and browserAPI
> before starting to load
> an URL.
For message manager, I understand there're frame scripts need to be loaded. But for browser API, I couldn't find things that parent needs child to do before loading the URL.
Even BrowserElementParent.js is loaded after sending URL, it will still receive the hello from BrowserElementChild.js since the message can only be handled in next event cycle. Are there anything I missed or misunderstood?
Comment 32•10 years ago
|
||
I think there was something in BrowserElementParent.js or in relevant scripts which sent a
message to the child when loaded.
Assignee | ||
Comment 33•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #32)
> I think there was something in BrowserElementParent.js or in relevant
> scripts which sent a
> message to the child when loaded.
Kanru / Fabrice, do you know what could be the message that child needs to handle before start loading which is related to browser API?
Flags: needinfo?(kchen)
Flags: needinfo?(fabrice)
Assignee | ||
Comment 34•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #32)
> I think there was something in BrowserElementParent.js or in relevant
> scripts which sent a
> message to the child when loaded.
I found only a hello from BrowserElementChild.js to BrowserElementParent.js when it's loaded, and it's the first message between them.
Kanru / Fabrice, is my understanding correct?
Comment 35•10 years ago
|
||
Yes, BrowserElementParent.js should always load before BrowserElementChild.js in order to receive the hello message.
Flags: needinfo?(kchen)
Assignee | ||
Comment 36•10 years ago
|
||
Yes, I understand this.
What I want to clarify is do we need to load BrowserElementParent.js before sending URL to child. My understanding is we can load it after sending since both loading BrowserElementParent.js and sendind URL are within nsFrameLoader::ReallyStartLoadingInternal(), which no matter it is loaded before/after sending the URL, it will be done before the hello message which is processed at later event cycle.
So if this is correct, I can send the URL before initializing Browser API and let content process to receive the URL ~10ms earlier.
Comment 37•10 years ago
|
||
Yes, I think so.
Comment 38•10 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #37)
> Yes, I think so.
I agree.
Flags: needinfo?(fabrice)
Assignee | ||
Comment 39•10 years ago
|
||
Flags: needinfo?(bugs)
Assignee | ||
Comment 40•10 years ago
|
||
Comment on attachment 8572530 [details] [diff] [review]
patch v4
Ask Kanru for another review since the reason in comment 27 is clarified and Olli has review queue too long.
Flags: needinfo?(bugs)
Attachment #8572530 -
Flags: review- → review?(kchen)
Comment 41•10 years ago
|
||
Comment on attachment 8572530 [details] [diff] [review]
patch v4
Review of attachment 8572530 [details] [diff] [review]:
-----------------------------------------------------------------
It looks like we could InitializeBrowserAPI earlier, right after we create the message manager. Currently InitializeBrowserAPI is buried inside ShowRemoteFrame, which is worrisome.
I suspect that the Webapps:RegisterBEP message is dead code. I'm not sure if it worked at all. The message direction seems not correct.
Attachment #8572530 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 42•10 years ago
|
||
Rebased and addressed comment 26.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c56e69766af1
(In reply to Kan-Ru Chen [:kanru] from comment #41)
> Comment on attachment 8572530 [details] [diff] [review]
> patch v4
>
> Review of attachment 8572530 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> It looks like we could InitializeBrowserAPI earlier, right after we create
> the message manager. Currently InitializeBrowserAPI is buried inside
> ShowRemoteFrame, which is worrisome.
Yes, we can InitializeBrowserAPI() earlier, but I'd like to do it after LoadURL() so we can send the URL ~10ms earlier. We can move it out from ShowRemoteFrame() in a followup bug.
Attachment #8572530 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 43•10 years ago
|
||
Keywords: checkin-needed
Comment 44•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•