1138749 - talos: TART, CART broken on e10s

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Avi Halachmi (:avih)]

+++ This bug was initially created as a clone of Bug #1134238 +++

Bug 1134238 broke TART and CART on e10s. As a result, the 's' (svgr) suit hangs and none if its results are reported.

mconley, I'd appreciate feedback on the comment - i.e. would every switch between URIs which end up as remote/non-remote also "reinitializes" the browser?

Comment 1

[Avi Halachmi (:avih)]

Attachment: bug1138749.patch

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8571690 [details] [diff] [review]
bug1138749.patch

Review of attachment 8571690 [details] [diff] [review]:
-----------------------------------------------------------------

this assumes that we have all chrome:// or all http:// files in a manifest.  While that is true given all our current tests, it is an assumption we are making.  I would like to mention in a comment something like:
If a manifest contains chrome:// and http:// or file:// uri's then e10s and remote tabs will not work


Of course the solution to this would be to open a new tab for each page, but that would affect our numbers and be a different test in general.

Comment 3

[Avi Halachmi (:avih)]

We discussed this on IRC and Joel agreed that the original comment does cover his concerns afteral.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8571690 [details] [diff] [review]
bug1138749.patch

Review of attachment 8571690 [details] [diff] [review]:
-----------------------------------------------------------------

::: talos/pageloader/chrome/pageloader.js
@@ +179,5 @@
>  
> +        function isExpectingFirstPageRemote() {
> +          // Among the first pages of the pageloader manifests, testing whether
> +          // or not the URI starts with 'chrome://' is enough to decide.
> +          return !pageUrls[0].toLowerCase().startsWith("chrome://");

I realize now that some chrome URIs can register themselves in their chrome manifests to load remotely: see bug 1083281.

You'll want to test that your first URI does not have the ability to load remotely if you want to go this route. I suggest using E10SUtils.jsm's canLoadURIInProcess method:

E10SUtils.canLoadURIInProcess(aURI, Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT);

Will return true if the URI can load in the content process.

Comment 5

[Avi Halachmi (:avih)]

Attachment: bug1138749.v2.patch

Carrying r+ since it's exactly the same code flow, but using the API which mconley(++!) recommends.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8572128 [details] [diff] [review]
bug1138749.v2.patch

Review of attachment 8572128 [details] [diff] [review]:
-----------------------------------------------------------------

One nit.

::: talos/pageloader/chrome/pageloader.js
@@ +199,2 @@
>          if (browserWindow.gMultiProcessBrowser) {
> +          if (firstPageCanLoadAsRemote())

Nit - might as well just && these instead of nesting them.

Comment 7

[Avi Halachmi (:avih)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6)
> Nit - might as well just && these instead of nesting them.

This was my initial approach, but I ended up with the current approach because this way the implicit else has a proper case.

If we && them, then there's no logical place for the (implicit) else.

Comment 8

[Avi Halachmi (:avih)]

https://hg.mozilla.org/build/talos/rev/64f3fd204a9a