781725 - Pre-fetch and precompile BrowserElementChild.js

Status: RESOLVED FIXED

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This process takes about 100ms in current code.  Short of juggling JS options, which will probably *increase* compilation time, we should just get this off the critical path.

Playing off of bug 781724, we can preload and precompile BrowserElementChild.js in the intervening time between process launch and loading the first PBrowser.

Since this is currently on the critical path, I expect all the time be knocked off startup, but I haven't written the patch to measure yet.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: WIP

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Refactor TabChild to allow pre-created instances, and then use a pre-created instance to pre-load and compile BrowserElementChild.js

There are a few things going on in this patch.  Sorry, couldn't split into smaller chunks, they're all interdependent.

This patch builds up several smaller parts in order to let us achieve this goal
 - when a content process is prelaunched, pre-fetch and pre-compile BrowserElementChild.js.
 - incidentally, also pre-create a TabChild instance to get JSContext etc. startup off the critical path

We need to create a TabChild instance in order to prefetch and precompile BrowserElementChild.js.  Here are the sub-parts

First I've split nsFrameMessageManager script-loading into separate fetch-and-compile and execute steps.  This is pretty trivial.

Second, TabChild now initializes its nsWebBrowser from Init().  That means we create a fake widget for it and so forth.  We need this initialization in order to initialize the TabChildGlobal.  This also fixes the bug dup'd to this one, something jlebar stepped on with the browser-element work.

Third, we aggressively initialize TabChildGlobal.  I added a flag requesting whether or not to execute BrowserElementChild.js when appropriate.  For precreated TabChild, we don't execute immediately.  Whenever we defer execution though, we always ensure BrowserElementChild.js has been executed before needed.

Fourth, we delay initialization of IME state until where we previously initialized the widget et al., from RecvShow().  Initializing IME requires an IPC connection, which we don't have for precreated TabChild.

And finally, I put all this together in TabChild::PreloadSlowThings.

Let me know if you have further questions or there are parts here you'd like someone else to review.

Comment 4

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

Comment on attachment 651867 [details] [diff] [review]
Refactor TabChild to allow pre-created instances, and then use a pre-created instance to pre-load and compile BrowserElementChild.js

> TabChild::TabChild(PRUint32 aChromeFlags, bool aIsBrowserElement,
>                    PRUint32 aAppId)
>   : mRemoteFrame(nullptr)
>   , mTabChildGlobal(nullptr)
>   , mChromeFlags(aChromeFlags)
>   , mOuterRect(0, 0, 0, 0)
>   , mLastBackgroundColor(NS_RGB(255, 255, 255))
>-  , mDidFakeShow(false)
>+  , mAppId(aAppId)
>   , mIsBrowserElement(aIsBrowserElement)
>-  , mAppId(aAppId)
>+  , mTriedBrowserInit(false)
> {
>     printf("creating %d!\n", NS_IsMainThread());
> }
(At some point we should remove those printfs)



> nsresult
> TabChild::Init()
> {
>-  nsCOMPtr<nsIWebBrowser> webBrowser = do_CreateInstance(NS_WEBBROWSER_CONTRACTID);
>-  if (!webBrowser) {
>-    NS_ERROR("Couldn't create a nsWebBrowser?");
>-    return NS_ERROR_FAILURE;
>-  }
>+    nsCOMPtr<nsIWebBrowser> webBrowser =
>+        do_CreateInstance(NS_WEBBROWSER_CONTRACTID);
>+    if (!webBrowser) {
>+        NS_ERROR("Couldn't create a nsWebBrowser?");
>+        return NS_ERROR_FAILURE;
>+    }
> 
>-  webBrowser->SetContainerWindow(this);
>-  mWebNav = do_QueryInterface(webBrowser);
>-  NS_ASSERTION(mWebNav, "nsWebBrowser doesn't implement nsIWebNavigation?");
>+    webBrowser->SetContainerWindow(this);
>+    mWebNav = do_QueryInterface(webBrowser);
>+    NS_ASSERTION(mWebNav, "nsWebBrowser doesn't implement nsIWebNavigation?");
> 
>-  nsCOMPtr<nsIDocShellTreeItem> docShellItem(do_QueryInterface(mWebNav));
>-  docShellItem->SetItemType(nsIDocShellTreeItem::typeContentWrapper);
>+    nsCOMPtr<nsIDocShellTreeItem> docShellItem(do_QueryInterface(mWebNav));
>+    docShellItem->SetItemType(nsIDocShellTreeItem::typeContentWrapper);
> 
>-  nsCOMPtr<nsIObserverService> observerService =
>-    do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
>+    nsCOMPtr<nsIObserverService> observerService =
>+        do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
This is somewhat odd. The whole file is for some reason using non-Gecko-coding-style indentation.
I'd prefer to change the code which uses wrong coding style to use right one, not other way around.
But for this patch, I'd just remove coding style changes.


>@@ -274,22 +276,36 @@ protected:
>     nsEventStatus DispatchWidgetEvent(nsGUIEvent& event);
> 
>     virtual PIndexedDBChild* AllocPIndexedDB(const nsCString& aASCIIOrigin,
>                                              bool* /* aAllowed */);
> 
>     virtual bool DeallocPIndexedDB(PIndexedDBChild* aActor);
> 

>+    enum FrameScriptLoading { DONT_LOAD_SCRIPTS, DEFAULT_LOAD_SCRIPTS };
>+    bool InitTabChildGlobal(FrameScriptLoading aScriptLoading=DEFAULT_LOAD_SCRIPTS);
Space before and after =

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 651867 [details] [diff] [review]
> Refactor TabChild to allow pre-created instances, and then use a pre-created
> instance to pre-load and compile BrowserElementChild.js
> 
> > TabChild::TabChild(PRUint32 aChromeFlags, bool aIsBrowserElement,
> >                    PRUint32 aAppId)
> >   : mRemoteFrame(nullptr)
> >   , mTabChildGlobal(nullptr)
> >   , mChromeFlags(aChromeFlags)
> >   , mOuterRect(0, 0, 0, 0)
> >   , mLastBackgroundColor(NS_RGB(255, 255, 255))
> >-  , mDidFakeShow(false)
> >+  , mAppId(aAppId)
> >   , mIsBrowserElement(aIsBrowserElement)
> >-  , mAppId(aAppId)
> >+  , mTriedBrowserInit(false)
> > {
> >     printf("creating %d!\n", NS_IsMainThread());
> > }
> (At some point we should remove those printfs)
> 
> 
> 
> > nsresult
> > TabChild::Init()
> > {
> >-  nsCOMPtr<nsIWebBrowser> webBrowser = do_CreateInstance(NS_WEBBROWSER_CONTRACTID);
> >-  if (!webBrowser) {
> >-    NS_ERROR("Couldn't create a nsWebBrowser?");
> >-    return NS_ERROR_FAILURE;
> >-  }
> >+    nsCOMPtr<nsIWebBrowser> webBrowser =
> >+        do_CreateInstance(NS_WEBBROWSER_CONTRACTID);
> >+    if (!webBrowser) {
> >+        NS_ERROR("Couldn't create a nsWebBrowser?");
> >+        return NS_ERROR_FAILURE;
> >+    }
> > 
> >-  webBrowser->SetContainerWindow(this);
> >-  mWebNav = do_QueryInterface(webBrowser);
> >-  NS_ASSERTION(mWebNav, "nsWebBrowser doesn't implement nsIWebNavigation?");
> >+    webBrowser->SetContainerWindow(this);
> >+    mWebNav = do_QueryInterface(webBrowser);
> >+    NS_ASSERTION(mWebNav, "nsWebBrowser doesn't implement nsIWebNavigation?");
> > 
> >-  nsCOMPtr<nsIDocShellTreeItem> docShellItem(do_QueryInterface(mWebNav));
> >-  docShellItem->SetItemType(nsIDocShellTreeItem::typeContentWrapper);
> >+    nsCOMPtr<nsIDocShellTreeItem> docShellItem(do_QueryInterface(mWebNav));
> >+    docShellItem->SetItemType(nsIDocShellTreeItem::typeContentWrapper);
> > 
> >-  nsCOMPtr<nsIObserverService> observerService =
> >-    do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
> >+    nsCOMPtr<nsIObserverService> observerService =
> >+        do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
> This is somewhat odd. The whole file is for some reason using
> non-Gecko-coding-style indentation.

The local style is 4-space indent.  Patches landed with 2-space indent, despite the modelines, which led to the mismatched style.  Functions added which ignored the modeline are very hard to hack on in editors that honor the modeline.

> But for this patch, I'd just remove coding style changes.
> 

Fine.

> 
> >@@ -274,22 +276,36 @@ protected:
> >     nsEventStatus DispatchWidgetEvent(nsGUIEvent& event);
> > 
> >     virtual PIndexedDBChild* AllocPIndexedDB(const nsCString& aASCIIOrigin,
> >                                              bool* /* aAllowed */);
> > 
> >     virtual bool DeallocPIndexedDB(PIndexedDBChild* aActor);
> > 
> 
> >+    enum FrameScriptLoading { DONT_LOAD_SCRIPTS, DEFAULT_LOAD_SCRIPTS };
> >+    bool InitTabChildGlobal(FrameScriptLoading aScriptLoading=DEFAULT_LOAD_SCRIPTS);
> Space before and after =

Changed.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is failing on try, on the in-process(!!!) dom/browser-element tests, apparently because of

10 INFO TEST-START | /tests/dom/browser-element/mochitest/test_browserElement_inproc_Alert.html
[snip]
JavaScript error: chrome://browser/content/tabbrowser.xml, line 1988: aTab is null

but nothing else suspicious around.  That means it must be the nsFrameMessageManager change, but I have absolutely no clue what the bug might be ... :/

Comment 7

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

Oh, you break data: url scripts

Comment 8

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

Or, hmm, maybe not.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Which is

      <method name="getBrowserForTab">
        <parameter name="aTab"/>
        <body>
        <![CDATA[
          return aTab.linkedBrowser;

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Olli Pettay [:smaug] from comment #7)
> Oh, you break data: url scripts

Feels like a GC / ownership problem.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Although yeah you're right,

  var script = 'data:,\
    testState = 0; \
    content.alert("Hello, world!"); \
    testState = 1; \
  ';

looks pretty darned suspicious ...

Comment 12

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

Comment on attachment 651867 [details] [diff] [review]
Refactor TabChild to allow pre-created instances, and then use a pre-created instance to pre-load and compile BrowserElementChild.js


>@@ -851,17 +860,16 @@ nsFrameScriptExecutor::LoadFrameScriptIn
>           if (!scheme.EqualsLiteral("data")) {
>             nsFrameJSScriptExecutorHolder* holder =
>               new nsFrameJSScriptExecutorHolder(script);
>             // Root the object also for caching.
>             JS_AddNamedScriptRoot(mCx, &(holder->mScript),
>                                   "Cached message manager script");
>             sCachedScripts->Put(aURL, holder);
>           }
>-          (void) JS_ExecuteScript(mCx, global, script, nullptr);
Ah this part.
Could become
else {
  (void) JS_ExecuteScript(mCx, global, script, nullptr);
}

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Followup fix: un-break data: frame scripts

Yep, it was data:!  Thanks for the quick pointer.

There's an OOP test failing now but it seems unrelated.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Reverting the fake show stuff made the failure go away :(.  Now seeing another OOP failure with use-after-__delete__, but might be able to kill 763602 after all ...

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This patch ended up being a barrel of monkeys.  I fixed up three more bugs, going through try now.  Will probably need review one of the bugfixes.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: 781725, followup: don't register for UI-only events until we have rendering state, and always destroy the window on shutdown.

The first set of fixes here restored jlebar's FakeShow() hack, which is still necessary.  Not requesting review on that.

This fixes a problem in which TabChild still alive around XPCOM shutdown would not destroy their DOM window state properly.  I don't know why this is happening with these patches, but it's a problem that could have happened without them.

Comment 17

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

Comment on attachment 655943 [details] [diff] [review]
781725, followup: don't register for UI-only events until we have rendering state, and always destroy the window on shutdown.

I'm not too familiar with "cancel-default-pan-zoom" and "browser-zoom-to-rect"
(and looks like those are pretty horrible notifications), but this change
doesn't make the situation any worse.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3925750b682f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3925750b682f