Closed
Bug 1138749
Opened 9 years ago
Closed 8 years ago
talos: TART, CART broken on e10s
Categories
(Testing :: Talos, defect)
Testing
Talos
Tracking
(e10s+)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: avih, Unassigned)
References
Details
Attachments
(1 file, 1 obsolete file)
3.02 KB,
patch
|
avih
:
review+
mconley
:
feedback+
|
Details | Diff | Splinter Review |
+++ 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?
Reporter | ||
Comment 1•9 years ago
|
||
Attachment #8571690 -
Flags: review?(jmaher)
Attachment #8571690 -
Flags: feedback?(mconley)
Comment 2•9 years ago
|
||
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+
Reporter | ||
Comment 3•9 years ago
|
||
We discussed this on IRC and Joel agreed that the original comment does cover his concerns afteral.
Comment 4•9 years ago
|
||
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-
Reporter | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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+
Reporter | ||
Comment 7•9 years ago
|
||
(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.
Reporter | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/build/talos/rev/64f3fd204a9a
![]() |
||
Updated•9 years ago
|
tracking-e10s:
--- → +
Reporter | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•