1190245 - Some more refactoring of nsFrameLoader

Status: RESOLVED FIXED

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

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Make the flow of DocShell creation / FrameScript loading / BrowserAPI initialization more clear

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 1, Separate mMessageManager creation and FrameScript loading

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 2, Make the MaybeCreateDocShell using code path easier to follow

Don't rely on MaybeCreateDocShell to set mRemoteFrame so we don't have
to call MaybeCreateDocShell in weird places. Instead we set mRemoteFrame
early in the nsFrameLoader constructor. This should still allow us to
switch default remoteness dynamically.

Should we get rid of the "MaybeCreateDocShell succeeded, but null mDocShell" assertion?

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Part 3, Make frame script loading and browser-api initialization easier to follow

Load frame script and initialize browser-api at the end of (1) DocShell
creation (2) RemoteBrowser creation (3) SetRemoteBrowser

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56650e09dd4a

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8642243 [details] [diff] [review]
Part 2, Make the MaybeCreateDocShell using code path easier to follow

>+nsFrameLoader::IsRemoteFrame()
>+{
>+  if (mRemoteFrame) {
>+    NS_ASSERTION(!mDocShell, "Find a remote frame with a DocShell");
s/Find/Found/ ?
I wonder if this could be even MOZ_ASSERT so that we'd get a crash on debug builds if this ever happens

> nsFrameLoader::CheckForRecursiveLoad(nsIURI* aURI)
> {
>   nsresult rv;
> 
>+  NS_ASSERTION(!IsRemoteFrame(),
>+               "Shouldn't call CheckForRecursiveLoad on remote frames.");
MOZ_ASSERT here



Thanks for doing this.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8642244 [details] [diff] [review]
Part 3, Make frame script loading and browser-api initialization easier to follow

>-  InitializeBrowserAPI();
>-  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>-  if (os) {
>-    os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
>-                        "inprocess-browser-shown", nullptr);
>-  }
>-
>-  if (OwnerIsBrowserOrAppFrame() && mMessageManager) {
>-    mMessageManager->LoadFrameScript(
>-      NS_LITERAL_STRING("chrome://global/content/BrowserElementChild.js"),
>-      /* allowDelayedLoad = */ true,
>-      /* aRunInGlobalScope */ true);
>+  if (OwnerIsBrowserOrAppFrame()) {
>     // For inproc frames, set the docshell properties.
>     nsCOMPtr<nsIDocShellTreeItem> item = do_GetInterface(docShell);
>     nsAutoString name;
>     if (mOwnerContent->GetAttr(kNameSpaceID_None, nsGkAtoms::name, name)) {
>       item->SetName(name);
>     }
>     mDocShell->SetFullscreenAllowed(
>       mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::allowfullscreen) ||
>@@ -1854,16 +1835,25 @@ nsFrameLoader::MaybeCreateDocShell()
>           nullptr);
>       } else {
>         nsCOMPtr<nsILoadContext> context = do_GetInterface(mDocShell);
>         context->SetUsePrivateBrowsing(true);
>       }
>     }
>   }
> 
>+  ReallyLoadFrameScripts();
>+  InitializeBrowserAPI();
>+
>+  nsCOMPtr<nsIObserverService> os = services::GetObserverService();
>+  if (os) {
>+    os->NotifyObservers(NS_ISUPPORTS_CAST(nsIFrameLoader*, this),
>+                        "inprocess-browser-shown", nullptr);
>+  }
So you change ordering here by moving BrowserElementChild.js loading to happen in InitializeBrowserAPI(), so
that script is now loaded before inprocess-browser-shown.
But I think that change makes sense

>@@ -2233,17 +2223,20 @@ nsFrameLoader::TryRemoteBrowser()
> 
>   if (mOwnerContent->AttrValueIs(kNameSpaceID_None,
>                                  nsGkAtoms::mozpasspointerevents,
>                                  nsGkAtoms::_true,
>                                  eCaseMatters)) {
>     unused << mRemoteBrowser->SendSetUpdateHitRegion(true);
>   }
> 
>-  return true;
>+  ReallyLoadFrameScripts();
>+  InitializeBrowserAPI();
>+
This also changes behavior, since currently nsFrameLoader::ShowRemoteFrame ends up
calling mRemoteBrowser->Show(size, parentIsActive); before EnsureMessageManager.
But, ReallyStartLoading should be in practice be called before ShowRemoteFrame, so again I think the change makes sense.



> nsFrameLoader::SetRemoteBrowser(nsITabParent* aTabParent)
> {
>   MOZ_ASSERT(!mRemoteBrowser);
>   mRemoteFrame = true;
>   mRemoteBrowser = TabParent::GetFrom(aTabParent);
>   mChildID = mRemoteBrowser ? mRemoteBrowser->Manager()->ChildID() : 0;
>+  ReallyLoadFrameScripts();
>+  InitializeBrowserAPI();
>   ShowRemoteFrame(ScreenIntSize(0, 0));

Do you need the method calls here? ShowRemoteFrame should call TryRemoteBrowser
Answer to this and remove the method calls if possible, and then r+

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8642244 [details] [diff] [review]
> Part 3, Make frame script loading and browser-api initialization easier to
> follow
> 
> > nsFrameLoader::SetRemoteBrowser(nsITabParent* aTabParent)
> > {
> >   MOZ_ASSERT(!mRemoteBrowser);
> >   mRemoteFrame = true;
> >   mRemoteBrowser = TabParent::GetFrom(aTabParent);
> >   mChildID = mRemoteBrowser ? mRemoteBrowser->Manager()->ChildID() : 0;
> >+  ReallyLoadFrameScripts();
> >+  InitializeBrowserAPI();
> >   ShowRemoteFrame(ScreenIntSize(0, 0));
> 
> Do you need the method calls here? ShowRemoteFrame should call TryRemoteBrowser
> Answer to this and remove the method calls if possible, and then r+

ShowRemoteFrame doesn't call TryRemoteBrowser because we already have mRemoteBrowser
assigned. The old code ends up calling ReallyLoadFrameScripts and InitializeBrowserAPI
in ShowRemoteFrame which looks very strange to me so I moved it to this place. So now
we load frame scripts after mRemoteBrowser is created or assigned consistently.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e775cd17d96
https://hg.mozilla.org/integration/mozilla-inbound/rev/3431c9521f12
https://hg.mozilla.org/integration/mozilla-inbound/rev/28708429fd23

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e775cd17d96
https://hg.mozilla.org/mozilla-central/rev/3431c9521f12
https://hg.mozilla.org/mozilla-central/rev/28708429fd23