+++ This bug was initially created as a clone of Bug #1344465 +++ Breaking this out from bug 1344465, because I'm going to flip the pref to allow the top level load of http pages (and form POSTs) in the file content process. I added this pref because of concerns that we might hit edge case compatibility issues, but this form POST problem seems a big enough one to mean we should ship initially with it on. I could special case just form POST to be done in the same process, but I think having POST and GET behaving differently is likely to be more confusing.

Does this need to be fixed before 53 release (on 53?)

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #1) > Does this need to be fixed before 53 release (on 53?) No this only occurs when the pref browser.tabs.remote.separateFileUriProcess is true, which is only on Nightly.

Is the idea here that when a |file://| page has |<form method=post action=http://>...|, when the form is submitted we should do that HTTP in the file content process?

Chrome's current behaviour seems to be to load http(s) pages (not just POST) in the same process as the file:// page, when it is something initiated by the content. That is via window.open, link, location change, form post, etc. It also stays in the same process if a same-origin navigation is made via the URL bar. History navigation honours this, even after process switch. My plan is to give very similar behaviour for Firefox, to try and limit any compatibility issues. The only difference will be that after process switch history entries will then load in their normal remote type. This saves us trying to store which remote type a history entry loaded in, in the parent, doing this doesn't seem worth it as we currently lose things like window references and history entry post data on process switch anyway. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dd69d222e0ddbe16acc9b4ca82375b59175c1b0 Earlier cross platform try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27716cdf3ffd0bca96d1fc74c0a21254d4a80187

This flag is for when we've loaded a URI in a remote type that is not the default for compatibility reasons (for example related http in the file content process). So that we can load the history entry in that same process as well.

This change means that any related http pages driven through content (window.open, links, etc.) will continue to be loaded in the file content process. Same-origin loads via the UI will also remain in the file content process. Cross-origin loads via the UI will cause a process switch. History navigation will stay in the process, if it was originally loaded in that process. The logic in the parent for swapping in and out of the Large Allocation process is also moved into browser.js RedirectLoad to fit in with the above changes. This also fixes a bug where we swap out of the Large Allocation process, when reloaded via the UI.

This ensures that we never get a short-lived process for the new tab, which can happen if we are not at the maximum for that remote type. We also set the correct remoteType.

This prevents short-lived processes when we are not at the maximum normal content process count and the new window's first tab is not going to load in that remote type.

(In reply to Bob Owen (:bobowen) from comment #5) > Chrome's current behaviour seems to be to load http(s) pages (not just POST) > in the same process as the file:// page, when it is something initiated by > the content. > That is via window.open, link, location change, form post, etc. > > It also stays in the same process if a same-origin navigation is made via > the URL bar. > > History navigation honours this, even after process switch. > > My plan is to give very similar behaviour for Firefox, to try and limit any > compatibility issues. > > The only difference will be that after process switch history entries will > then load in their normal remote type. AIUI this still means that if we end up loading an http page from a (link on a) file page, we end up opening the http page in the file process, which will constrain what we can do to sandbox those pages... Why can we not process-switch in those cases? I would expect the same origin policy to mean that the file: and http: pages would never know about each other anyway, so beyond serializing and deserializing the POST content, I don't understand why we wouldn't do a process switch. Can you elaborate on the problems you would see with doing this?

(In reply to :Gijs from comment #14) > (In reply to Bob Owen (:bobowen) from comment #5) > > Chrome's current behaviour seems to be to load http(s) pages (not just POST) > > in the same process as the file:// page, when it is something initiated by > > the content. > > That is via window.open, link, location change, form post, etc. > > > > It also stays in the same process if a same-origin navigation is made via > > the URL bar. > > > > History navigation honours this, even after process switch. > > > > My plan is to give very similar behaviour for Firefox, to try and limit any > > compatibility issues. > > > > The only difference will be that after process switch history entries will > > then load in their normal remote type. > > AIUI this still means that if we end up loading an http page from a (link on > a) file page, we end up opening the http page in the file process, which > will constrain what we can do to sandbox those pages... Why can we not > process-switch in those cases? I would expect the same origin policy to mean > that the file: and http: pages would never know about each other anyway, so > beyond serializing and deserializing the POST content, I don't understand > why we wouldn't do a process switch. Can you elaborate on the problems you > would see with doing this? Well, if we process switch then we lose all references (like opener and the opened window from window.open), so everything breaks even the things you can do cross-origin. This includes the tab group things, like named browsing contexts (see bug 1359021). So, there are definitely potential compatibility issues, which is why I'm erring on the side of caution. Thinking about it, I could file a follow-up to see how often we get top level http loads in the file content process. The suspicion is that file:// URIs tend to be used more in enterprise settings, so the telemetry might not be conclusive I suppose, but still useful. Might as well include framed loads as well.

Comment on attachment 8864114 [details] [diff] [review] Part 2: Limit the http pages that will load in the file content process with allowLinkedWebInFileUriProcess pref Review of attachment 8864114 [details] [diff] [review]: ----------------------------------------------------------------- This mostly looks sane but I still have questions. I also wonder if :mconley or :mikedeboer is a better reviewer here... ::: browser/base/content/browser.js @@ +1079,5 @@ > function RedirectLoad({ target: browser, data }) { > + if (data.loadOptions.reloadInFreshProcess) { > + // Convert the fresh process load option into a large allocation remote type > + // to use common processing from this point. > + data.loadOptions.remoteType = E10SUtils.LARGE_ALLOCATION_REMOTE_TYPE; Why is this always the desired process when using reloadInFreshProcess? Seems like the reloadInFreshProcess flag is kind of misnamed if 'fresh' always means 'large allocation'... ::: browser/modules/E10SUtils.jsm @@ +102,5 @@ > } > > let uri; > try { > + uri = Services.uriFixup.createFixupURI(aURL, Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS); This is a pretty significant change... why do we need to do this? At some point we should change a bunch of the browser frontend code to stop fixing things up all over the place and have a clear boundary of where we expect valid URLs and where input is an arbitrary string. The current mishmash is unfortunate. :-(

Comment on attachment 8864115 [details] [diff] [review] Part 3: In tabbrowser.xml adoptTab, make sure the new tab is in the same process as adoptee Review of attachment 8864115 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tabbrowser.xml @@ -3579,5 @@ > - // If we're an e10s browser window, an exception will be thrown > - // if we attempt to drag a non-remote browser in, so we need to > - // ensure that the remoteness of the newly created browser is > - // appropriate for the URL of the tab being dragged in. > - this.updateBrowserRemotenessByURL(newBrowser, newURL); This looks like dragging tabs from e10s to non-e10s windows will now just break (or be very confusing for the user, because they can end up with parent process browsers in their e10s window, or remote browsers in their non-e10s window). Am I missing something? Given that creating explicitly e10s/non-e10s windows is a nightly testing feature only, perhaps we could just catch those specific cases and throw up a modal alert to warn the user why this didn't work, or something? Or specialcase them to force the 'right' process to be used...

Comment on attachment 8864117 [details] [diff] [review] Part 4: Select correct remote type upfront when passed window arguments in browser.js onDCL Review of attachment 8864117 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +1129,5 @@ > document.getAnonymousElementByAttribute(gBrowser, "anonid", "initialBrowser"); > > + let isRemote = gMultiProcessBrowser; > + let remoteType; > + let sameProcessAsFrameLoader; Please ensure these are initialized to whatever the default ends up being when they get passed to updateBrowserRemoteness() if there are no window.arguments (or we get passed a tab without a linkedBrowser). @@ +1142,5 @@ > + argToLoad.getAttribute("usercontextid")); > + } > + > + let linkedBrowser = argToLoad.linkedBrowser; > + if (linkedBrowser) { When is this null/falsy? Can you add a comment?

Comment on attachment 8864118 [details] [diff] [review] Part 5: Enable pref allowLinkedWebInFileUriProcess to allow related http(s) content to top level load in file content process Review of attachment 8864118 [details] [diff] [review]: ----------------------------------------------------------------- rs=me

Comment on attachment 8864119 [details] [diff] [review] Part 6: Include more tests cases for allowLinkedWebInFileUriProcess pref Review of attachment 8864119 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/tabs/browser_new_web_tab_in_file_process_pref.js @@ +53,5 @@ > + if (httpTab != gBrowser.selectedTab) { > + httpTab = yield BrowserTestUtils.switchTab(gBrowser, httpTab); > + httpBrowser = httpTab.linkedBrowser; > + } > + let promiseLoad = BrowserTestUtils.browserLoaded(httpBrowser); This races, because up to this point you haven't waited for the tab to load, only for it to open, so this promise might resolve because of the previous load rather than the new one (after the reload). You probably want to do something like: yield ContentTask.spawn(httpBrowser, TEST_HTTP, function*(uri) { if (content.document.readyState != "complete" || content.document.documentURIObject.scheme != "http") { return ContentTaskUtils.waitForEvent(this, "DOMContentLoaded", true, () => content.document.documentURI == uri); } });

(In reply to :Gijs from comment #16) > ::: browser/base/content/browser.js > @@ +1079,5 @@ > > function RedirectLoad({ target: browser, data }) { > > + if (data.loadOptions.reloadInFreshProcess) { > > + // Convert the fresh process load option into a large allocation remote type > > + // to use common processing from this point. > > + data.loadOptions.remoteType = E10SUtils.LARGE_ALLOCATION_REMOTE_TYPE; > > Why is this always the desired process when using reloadInFreshProcess? > Seems like the reloadInFreshProcess flag is kind of misnamed if 'fresh' > always means 'large allocation'... It is misnamed, see bug 1334170 where I'm hoping to someday get around to fixing the naming

Comment on attachment 8864114 [details] [diff] [review] Part 2: Limit the http pages that will load in the file content process with allowLinkedWebInFileUriProcess pref Review of attachment 8864114 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure I understand why we're doing this change. The only time when we need to make sure that we don't leave the current content process when loading an http URI from a file:// URI is when we have other related windows, which is easy for us to check with `nsIDocShell::IsOnlyTopLevelInTabGroup`. In the other cases I would suggest always leaving the file process even if we're doing a same origin load via the UI. What is the motivation for staying in the file process in these cases? Does it break websites in some way? It also seems like this patch contains 2 mostly unrelated changes: the change to move the Large-Allocation check into RedirectLoad, and the change to cause loads of HTTP uris to occur within a file content process. ::: browser/base/content/browser.js @@ +1076,5 @@ > > // Called when a docshell has attempted to load a page in an incorrect process. > // This function is responsible for loading the page in the correct process. > function RedirectLoad({ target: browser, data }) { > + if (data.loadOptions.reloadInFreshProcess) { I do like that we've moved this logic for reloadInFreshProcess up to RedirectLoad. I think it works better at this point than it did in updateBrowserRemotenessByURL. ::: browser/modules/E10SUtils.jsm @@ +62,5 @@ > + if (aCurrentUri) { > + const sm = Services.scriptSecurityManager; > + try { > + // checkSameOriginURI throws when not same origin. > + sm.checkSameOriginURI(aCurrentUri, aTargetUri, false); I don't like that we're using exceptions for this. I don't suppose this is exposed any other way on nsIScriptSecurityManager? @@ +102,5 @@ > } > > let uri; > try { > + uri = Services.uriFixup.createFixupURI(aURL, Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS); I agree with :Gijs that this is pretty gross.

Comment on attachment 8864121 [details] [diff] [review] Part 8: Check that reload doesn't cause new process when large allocation browser in tab group Review of attachment 8864121 [details] [diff] [review]: ----------------------------------------------------------------- Thanks :)

(In reply to :Gijs from comment #16) > This mostly looks sane but I still have questions. I also wonder if :mconley > or :mikedeboer is a better reviewer here... I'll let you redirect if necessary. > ::: browser/modules/E10SUtils.jsm > @@ +102,5 @@ > > } > > > > let uri; > > try { > > + uri = Services.uriFixup.createFixupURI(aURL, Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS); > > This is a pretty significant change... why do we need to do this? We're not process switching for a UI driven navigation when the URI is same origin. Unfortunately, the URL we receive here seems to be pretty raw. So, if we are already on http://example.com and someone types in example.com, Services.io.newURI throws and we'll switch. It seems pretty odd to behave differently for http://example.com and example.com, as people would often not type the http://. As you say it will be great when we're passing around real URIs and this function can go completely. I've realised that I don't need the FIXUP_FLAG_FIX_SCHEME_TYPOS flag though, that does even more things that I think we can reasonable ignore here. (In reply to Michael Layzell [:mystor] from comment #22) > I'm not sure I understand why we're doing this change. The only time when we > need to make sure that we don't leave the current content process when > loading an http URI from a file:// URI is when we have other related > windows, which is easy for us to check with > `nsIDocShell::IsOnlyTopLevelInTabGroup`. In the other cases I would suggest > always leaving the file process even if we're doing a same origin load via > the UI. > > What is the motivation for staying in the file process in these cases? Does > it break websites in some way? We discussed this at some length on IRC and I've filed bug 1362331 to track always switching when we're the only top level in the tab group. > It also seems like this patch contains 2 mostly unrelated changes: the > change to move the Large-Allocation check into RedirectLoad, and the change > to cause loads of HTTP uris to occur within a file content process. Yeah, I tried to break this out before, but it was all tied up, but the patch simplified a bit since then, so it wasn't too painful. I'll upload the large allocation change after this. > ::: browser/modules/E10SUtils.jsm > @@ +62,5 @@ > > + if (aCurrentUri) { > > + const sm = Services.scriptSecurityManager; > > + try { > > + // checkSameOriginURI throws when not same origin. > > + sm.checkSameOriginURI(aCurrentUri, aTargetUri, false); > > I don't like that we're using exceptions for this. I don't suppose this is > exposed any other way on nsIScriptSecurityManager? I was pretty surprised there wasn't a more script friendly way, but I can't find one. > > + uri = Services.uriFixup.createFixupURI(aURL, Ci.nsIURIFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS); > > I agree with :Gijs that this is pretty gross. Then we are all in agreement. :-)

This fixes a bug where we swap out of the Large Allocation process, when reloaded via the UI.

Comment on attachment 8864115 [details] [diff] [review] Part 3: In tabbrowser.xml adoptTab, make sure the new tab is in the same process as adoptee (In reply to :Gijs from comment #17) > ::: browser/base/content/tabbrowser.xml > @@ -3579,5 @@ > > - // If we're an e10s browser window, an exception will be thrown > > - // if we attempt to drag a non-remote browser in, so we need to > > - // ensure that the remoteness of the newly created browser is > > - // appropriate for the URL of the tab being dragged in. > > - this.updateBrowserRemotenessByURL(newBrowser, newURL); > > This looks like dragging tabs from e10s to non-e10s windows will now just > break (or be very confusing for the user, because they can end up with > parent process browsers in their e10s window, or remote browsers in their > non-e10s window). Am I missing something? On Windows at least you don't seem to be able to drag tabs between e10s and non-e10s windows. We don't display the arrow to indicate where the tab will be inserted and it just opens in a new window instead. Not sure where we're doing that, but even if you did manage it addTab already calls getRemoteTypeForURI with the appropriate gMultiProcessBrowser for that window, so we would end up with the browser in the same process as the existing code, I think (just always right first time). We'd still get an exception either way in swapBrowsersAndCloseOther, because the remoteness wouldn't match.

(In reply to :Gijs from comment #18) > ::: browser/base/content/browser.js > @@ +1129,5 @@ > > document.getAnonymousElementByAttribute(gBrowser, "anonid", "initialBrowser"); > > > > + let isRemote = gMultiProcessBrowser; > > + let remoteType; > > + let sameProcessAsFrameLoader; > > Please ensure these are initialized to whatever the default ends up being > when they get passed to updateBrowserRemoteness() if there are no > window.arguments (or we get passed a tab without a linkedBrowser). If I understand correctly that's what it does, as the current code doesn't pass through the options object, so remoteType and sameProcessAsFrameLoader would be undefined on the default initialised object in updateBrowserRemoteness. > @@ +1142,5 @@ > > + argToLoad.getAttribute("usercontextid")); > > + } > > + > > + let linkedBrowser = argToLoad.linkedBrowser; > > + if (linkedBrowser) { > > When is this null/falsy? Can you add a comment? Well I expect it never should be, I was just being cautious because we've just pulled it out of the window arguments. I'll add a comment to that effect.

(In reply to :Gijs from comment #20) > Comment on attachment 8864119 [details] [diff] [review] > Part 6: Include more tests cases for allowLinkedWebInFileUriProcess pref > > Review of attachment 8864119 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: > browser/base/content/test/tabs/browser_new_web_tab_in_file_process_pref.js > @@ +53,5 @@ > > + if (httpTab != gBrowser.selectedTab) { > > + httpTab = yield BrowserTestUtils.switchTab(gBrowser, httpTab); > > + httpBrowser = httpTab.linkedBrowser; > > + } > > + let promiseLoad = BrowserTestUtils.browserLoaded(httpBrowser); > > This races, because up to this point you haven't waited for the tab to load, > only for it to open, so this promise might resolve because of the previous > load rather than the new one (after the reload). This seemed like a fairly common scenario, so I've added an option to BrowserTestUtils.waitForNewTab, so that it only resolves after the load. Defaulted it to false, so existing calls get the same beahviour. I've changed the test locally to pass true.

Comment on attachment 8864888 [details] [diff] [review] Part 2: Limit the http pages that will load in the file content process with allowLinkedWebInFileUriProcess pref Review of attachment 8864888 [details] [diff] [review]: ----------------------------------------------------------------- Eh, this looks fine then. Thanks for clarifying! ::: browser/modules/E10SUtils.jsm @@ +107,5 @@ > } > > let uri; > try { > + uri = Services.uriFixup.createFixupURI(aURL, Ci.nsIURIFixup.FIXUP_FLAG_NONE); Can you file a followup to basically convert callsites to do this earlier so we pass in a real URI instead of a string?

(In reply to Bob Owen (:bobowen) from comment #27) > (In reply to :Gijs from comment #18) > > > ::: browser/base/content/browser.js > > @@ +1129,5 @@ > > > document.getAnonymousElementByAttribute(gBrowser, "anonid", "initialBrowser"); > > > > > > + let isRemote = gMultiProcessBrowser; > > > + let remoteType; > > > + let sameProcessAsFrameLoader; > > > > Please ensure these are initialized to whatever the default ends up being > > when they get passed to updateBrowserRemoteness() if there are no > > window.arguments (or we get passed a tab without a linkedBrowser). > > If I understand correctly that's what it does, as the current code doesn't > pass through the options object, so remoteType and sameProcessAsFrameLoader > would be undefined on the default initialised object in > updateBrowserRemoteness. Right, what I meant was, if "undefined" is the default and the callee is supposed to cope, can we add a comment here? Then it's a little clearer that that's what ends up happening. I guess I thought that assigning undefined explicitly would help, but on further reflection I don't agree with my past self. A comment would do the job better. :-)

Comment on attachment 8864902 [details] [diff] [review] Part 6 prologue: Allow BrowserTestUtils.waitForNewTab to optionally wait for the page in the new tab to load Review of attachment 8864902 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm @@ +321,5 @@ > + if (loadedUrl != newLocation) { > + throw new Error(`Expected URL: ${newLocation} to load, but got ${loadedUrl}.`); > + } > + > + return newTab; Assuming this works in practice, r=me. This stuff can be... fiddly... to say the least. In particular, I don't know for sure if you're guaranteed to get 1 load (and only 1 load) after onLocationChange, or if it's possible for these messages to be dispatched in the opposite order. If this does end up being intermittent or something, I expect we'd want to create the browserLoaded() promise immediately when we receive the TabOpen event, and restrict it to resolving for the right URI (by passing it a third param).

Comment on attachment 8864888 [details] [diff] [review] Part 2: Limit the http pages that will load in the file content process with allowLinkedWebInFileUriProcess pref Review of attachment 8864888 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/E10SUtils.jsm @@ +90,5 @@ > return remoteType == this.getRemoteTypeForURI(aURL, true, remoteType); > }, > > getRemoteTypeForURI(aURL, aMultiProcess, > + aPreferredRemoteType = DEFAULT_REMOTE_TYPE, aCurrentUri, NIT: put aCurrentUri on a different line. @@ +112,3 @@ > } catch (e) { > // If we have an invalid URI, it's still possible that it might get > // fixed-up into a valid URI later on. However, we don't want to return Isn't this inaccurate with the new behavior above?

Comment on attachment 8864889 [details] [diff] [review] Part 2.5: Move logic in the parent for switching in and out of Large Allocation process into browser.js RedirectLoad Review of attachment 8864889 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/E10SUtils.jsm @@ +90,5 @@ > return remoteType == this.getRemoteTypeForURI(aURL, true, remoteType); > }, > > getRemoteTypeForURI(aURL, aMultiProcess, > + aPreferredRemoteType = DEFAULT_REMOTE_TYPE, aCurrentUri) { NIT: aCurrentUri on a new line (I think I ask for this on the other patch too)

Comment on attachment 8864112 [details] [diff] [review] Part 1: Add flag to nsISHEntry to indicate if it was originally loaded in this process r=me, but should there be session restore bits of some sort here too? I guess it's not clear what they would do, exactly....

Comment on attachment 8864120 [details] [diff] [review] Part 7: Check that we can form post from file:// URL to http:// URL r=me, I think; I'm not confident in my ability to spot bugs in the promise logic here.

Thanks the everyone for the reviews. (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #34) > Comment on attachment 8864112 [details] [diff] [review] > Part 1: Add flag to nsISHEntry to indicate if it was originally loaded in > this process > > r=me, but should there be session restore bits of some sort here too? I > guess it's not clear what they would do, exactly.... As I mentioned on IRC (and you say here), as the restore generally only happens when the browser is switched into a different process, it doesn't really make sense to save and restore this flag. (In reply to :Gijs from comment #29) > Comment on attachment 8864888 [details] [diff] [review] > Part 2: Limit the http pages that will load in the file content process with ... > > + uri = Services.uriFixup.createFixupURI(aURL, Ci.nsIURIFixup.FIXUP_FLAG_NONE); > > Can you file a followup to basically convert callsites to do this earlier so > we pass in a real URI instead of a string? Bug 1365535 filed. (In reply to Michael Layzell [:mystor] from comment #32) > Comment on attachment 8864888 [details] [diff] [review] > Part 2: Limit the http pages that will load in the file content process with ... > > getRemoteTypeForURI(aURL, aMultiProcess, > > + aPreferredRemoteType = DEFAULT_REMOTE_TYPE, aCurrentUri, > > NIT: put aCurrentUri on a different line. Fixed locally. > @@ +112,3 @@ > > } catch (e) { > > // If we have an invalid URI, it's still possible that it might get > > // fixed-up into a valid URI later on. However, we don't want to return > > Isn't this inaccurate with the new behavior above? It could still get fixed up as it's likely that later on we'll use more flags. I'm not using any here because we don't have the flags and the main one I'm concern about here is just missing http:// etc. (In reply to :Gijs from comment #30) > (In reply to Bob Owen (:bobowen) from comment #27) ... > Right, what I meant was, if "undefined" is the default and the callee is > supposed to cope, can we add a comment here? Then it's a little clearer that > that's what ends up happening. I guess I thought that assigning undefined > explicitly would help, but on further reflection I don't agree with my past > self. A comment would do the job better. :-) Comment added locally.

(In reply to :Gijs from comment #31) > Comment on attachment 8864902 [details] [diff] [review] > Part 6 prologue: Allow BrowserTestUtils.waitForNewTab to optionally wait for > ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm > @@ +321,5 @@ > > + if (loadedUrl != newLocation) { > > + throw new Error(`Expected URL: ${newLocation} to load, but got ${loadedUrl}.`); > > + } > > + > > + return newTab; > > Assuming this works in practice, r=me. This stuff can be... fiddly... to say > the least. In particular, I don't know for sure if you're guaranteed to get > 1 load (and only 1 load) after onLocationChange, or if it's possible for > these messages to be dispatched in the opposite order. > > If this does end up being intermittent or something, I expect we'd want to > create the browserLoaded() promise immediately when we receive the TabOpen > event, and restrict it to resolving for the right URI (by passing it a third > param). This seems like a much more robust approach, so I've made that change. As I'm a sucker for punishment that included some more refactoring, I'll upload so you take another look.

Comment on attachment 8868562 [details] [diff] [review] Part 6 prologue: Allow BrowserTestUtils.waitForNewTab to optionally wait for the page in the new tab to load Review of attachment 8868562 [details] [diff] [review]: ----------------------------------------------------------------- Nice lambda function reuse refactor, very much r=me. Thanks for tackling this! ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm @@ +303,5 @@ > + result = newTab; > + } else { > + // If waiting for load, resolve with promise for that, which when load > + // completes resolves to the new tab. > + result = BrowserTestUtils.browserLoaded(newBrowser, false, urlMatches) Nit: The OCD "death to double negations" part of me would use if (waitForLoad) { /* result is browserLoaded() */ } else { /* newTab */ } rather than this way around, because it is every such a tiny bit easier to read.

I had to do a fair bit of rebasing so I've done a linux only try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bf9d71e54074ec9a54f216c9d4ec61474b5f935 (In reply to :Gijs from comment #39) > Comment on attachment 8868562 [details] [diff] [review] ... > Nice lambda function reuse refactor, very much r=me. Thanks for tackling > this! Thanks. > ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm > @@ +303,5 @@ > > + result = newTab; > > + } else { > > + // If waiting for load, resolve with promise for that, which when load > > + // completes resolves to the new tab. > > + result = BrowserTestUtils.browserLoaded(newBrowser, false, urlMatches) > > Nit: The OCD "death to double negations" part of me would use if > (waitForLoad) { /* result is browserLoaded() */ } else { /* newTab */ } > rather than this way around, because it is every such a tiny bit easier to > read. Yeah, I was in two minds, I did it this way, because it meant the default behaviour was first. However, I think you're right, so I'm happy to switch it round.

(In reply to Bob Owen (:bobowen) from comment #40) > I had to do a fair bit of rebasing so I've done a linux only try push: > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=3bf9d71e54074ec9a54f216c9d4ec61474b5f935 bc failures there were because of the reload button name change. Don't think the QuantumRender mda test failures are related.

Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff171bde1493 Part 1: Add flag to nsISHEntry to indicate if it was originally loaded in this process. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/76014fefc55d Part 2: Limit the http pages that will load in the file content process with allowLinkedWebInFileUriProcess pref. r=Gijs, r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/34b454ccbf6c Part 2.5: Move logic in the parent for switching in and out of Large Allocation process into browser.js RedirectLoad. r=mystor https://hg.mozilla.org/integration/mozilla-inbound/rev/7450b84a62be Part 3: In tabbrowser.xml adoptTab, make sure the new tab is in the same process as adoptee. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/25062ff6368b Part 4: Select correct remote type upfront when passed window arguments in browser.js onDCL. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb9327b90a8 Part 5: Enable pref allowLinkedWebInFileUriProcess to allow related http(s) content to top level load in file content process. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7bb953acd0 Part 6 prologue: Allow BrowserTestUtils.waitForNewTab to optionally wait for the page in the new tab to load. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/34a407da29ea Part 6: Include more tests cases for allowLinkedWebInFileUriProcess pref. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/c249b6ccb0c7 Part 7: Check that we can form post from file:// URL to http:// URL. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/49e6ba180450 Part 8: Check that reload doesn't cause new process when large allocation browser in tab group. r=mystor

https://hg.mozilla.org/mozilla-central/rev/ff171bde1493 https://hg.mozilla.org/mozilla-central/rev/76014fefc55d https://hg.mozilla.org/mozilla-central/rev/34b454ccbf6c https://hg.mozilla.org/mozilla-central/rev/7450b84a62be https://hg.mozilla.org/mozilla-central/rev/25062ff6368b https://hg.mozilla.org/mozilla-central/rev/2bb9327b90a8 https://hg.mozilla.org/mozilla-central/rev/bd7bb953acd0 https://hg.mozilla.org/mozilla-central/rev/34a407da29ea https://hg.mozilla.org/mozilla-central/rev/c249b6ccb0c7 https://hg.mozilla.org/mozilla-central/rev/49e6ba180450

Hi Andrei, I was unable to find "Simona Marcu" so I thought I'd NI you. Could you please add this as a test case for the feature tracked by Bug 1147911? I wasn't sure if it's already in the test plan, thought it was worth pinging you about. Thanks!

(In reply to Ritu Kothari (:ritu) from comment #45) > Hi Andrei, I was unable to find "Simona Marcu" so I thought I'd NI you. > Could you please add this as a test case for the feature tracked by Bug > 1147911? I wasn't sure if it's already in the test plan, thought it was > worth pinging you about. Thanks! I see it in the test plan already. Glad it was covered!

Reproduced the initial issue using Nightly 55.0a1 (Build ID: 20170510030301) on Windows 10 x64. Verified as fixed using the latest Nightly 55.0a1 (Build ID: 20170522030207) on Windows 10 x64, Ubuntu 16.04 x64, Mac OS X 10.12.