Closed Bug 1121905 Opened 10 years ago Closed 9 years ago

Reorder things of remote frame loading to send child process the URL earlier

Categories

(Core :: DOM: Content Processes, defect)

Product:
Core
Component:
DOM: Content Processes
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: ting, Assigned: ting)

References

Details

Attachments

(1 file, 5 obsolete files)

patch v5
(deleted), 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.
Move remote-browser-pending after TryRemoteBrowser() could possibly miss the hello from BrowserElementChild.js.
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)
I don't know if he reads his bugmail.  You may want to email him directly.
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)
(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.  :)
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.
Attached patch patch v1 (obsolete) (deleted) — — Splinter Review 
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be740ba8961f

Still there're failed tests.
(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: nobody → tchou
Status: NEW → ASSIGNED
QA Contact: tchou
QA Contact: tchou
Attached patch patch v2 (obsolete) (deleted) — — Splinter Review 
Try looks better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf0b83ec917a
Attachment #8559647 - Attachment is obsolete: true
Blocks: 1110624
No longer blocks: AppStartup
Attached patch patch v2 (obsolete) (deleted) — — Splinter Review 
Rebased.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7942e7aee4f7
Attachment #8560327 - Attachment is obsolete: true
Attachment #8563281 - Flags: review?(bugs)
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-
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.
(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().
(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?
(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?
Attached patch patch v3 (obsolete) (deleted) — — Splinter Review 
(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 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-
Do you know why does EnsureMessageManager() initialize nsFrameMessageManager's mCallback after mRemoteBrowserShown is true?
Flags: needinfo?(bugs)
(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()?
(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?
(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)
(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...
(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.
Attached patch patch v4 (obsolete) (deleted) — — Splinter Review 
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 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 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-
No longer depends on: 1119692
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...
(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)
But we need to send the stuff related to message manager and browserAPI before starting to load
an URL.
Flags: needinfo?(bugs)
(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?
I think there was something in BrowserElementParent.js or in relevant scripts which sent a
message to the child when loaded.
(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)
(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?
Yes, BrowserElementParent.js should always load before BrowserElementChild.js in order to receive the hello message.
Flags: needinfo?(kchen)
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.
Yes, I think so.
(In reply to Kan-Ru Chen [:kanru] from comment #37)
> Yes, I think so.

I agree.
Flags: needinfo?(fabrice)
Per comment 33 ~ 38, are there still concerns for attachment 8572530 [details] [diff] [review]?
Flags: needinfo?(bugs)
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 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+
Attached patch patch v5 (deleted) — — Splinter Review 
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/714eed0515ff
Status: ASSIGNED → RESOLVED
Closed: 9 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.

Attachment

General

Creator:
Created:
Updated:
Size: