Closed Bug 1138749 Opened 10 years ago Closed 9 years ago

talos: TART, CART broken on e10s

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(e10s+)

RESOLVED FIXED
Tracking Status
e10s + ---

People

(Reporter: avih, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

+++ 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?
Attached patch bug1138749.patch (obsolete) — Splinter Review
Attachment #8571690 - Flags: review?(jmaher)
Attachment #8571690 - Flags: feedback?(mconley)
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.
Attachment #8571690 - Flags: review?(jmaher) → review+
We discussed this on IRC and Joel agreed that the original comment does cover his concerns afteral.
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.
Attachment #8571690 - Flags: feedback?(mconley) → feedback-
Carrying r+ since it's exactly the same code flow, but using the API which mconley(++!) recommends.
Attachment #8571690 - Attachment is obsolete: true
Attachment #8572128 - Flags: review+
Attachment #8572128 - Flags: feedback?(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.
Attachment #8572128 - Flags: feedback?(mconley) → feedback+
(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.
tracking-e10s: --- → +
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: