As far as I can tell, this was regressed by the check added in bug 1317293, as we try to swap a "webLargeAllocation" frameLoader with a "web" frameLoader.

MozReview-Commit-ID: LfavqjMcZUq

MozReview-Commit-ID: 6aF2wlLgy42

MozReview-Commit-ID: BHFgjwRGrge

MozReview-Commit-ID: XwwrAbeyhy

Comment on attachment 8836150 [details] [diff] [review] Part 2: Simplify the in large allocation process logic > function getInLAProc(aBrowser) { > return ContentTask.spawn(aBrowser, null, () => { >- try { >- return docShell.inLargeAllocProcess; >- } catch (e) { >- // This must be a non-remote browser, which means it is not fresh >- return false; >- } >+ return Services.appinfo.remoteType == "webLargeAllocation"; So some other patch adds remoteType?

Comment on attachment 8836151 [details] [diff] [review] Part 3: Relax the SwapWithOtherRemoteLoader swap check Review of attachment 8836151 [details] [diff] [review]: ----------------------------------------------------------------- I am not a reviewer for this area, maybe :bz is a good choice? ::: dom/base/nsFrameLoader.cpp @@ +1351,5 @@ > + if (!otherContent->GetAttr(kNameSpaceID_None, nsGkAtoms::RemoteType, > + otherRemoteType)) { > + otherRemoteType.AssignLiteral(DEFAULT_REMOTE_TYPE); > + } > + ourContent->SetAttr(kNameSpaceID_None, nsGkAtoms::RemoteType, otherRemoteType, false); These seems too early to set the attributes, since there are still many checks that could abort the swap. Maybe move this to end just before returning NS_OK?

Comment on attachment 8836152 [details] [diff] [review] Part 4: Add tests for dragging browsers into new windows remove debugger

(In reply to Olli Pettay [:smaug] from comment #5) > So some other patch adds remoteType? remoteType was added by bug 1147911. I just didn't take advantage of it properly for large allocation until part 1 of this bug.

MozReview-Commit-ID: BHFgjwRGrge

MozReview-Commit-ID: XwwrAbeyhy

Comment on attachment 8836149 [details] [diff] [review] Part 1: Use remoteType to propagate Large-Allocation status Review of attachment 8836149 [details] [diff] [review]: ----------------------------------------------------------------- I don't think I'm the right person to review the dom/ bits - they usually need the right peer. The changes do look sane to me :) I'm more interested in the JS/ ipc interaction. ::: browser/base/content/tabbrowser.xml @@ +1842,5 @@ > + // If we're in a LargeAllocation process, we prefer switching back > + // into a normal content process, as that way we can clean up the > + // L-A process. > + let preferredRemoteType = aBrowser.remoteType; > + if (aBrowser.remoteType == E10SUtils.LARGE_ALLOCATION_REMOTE_TYPE) { Can you rename this to use `preferredRemoteType` ? @@ +1846,5 @@ > + if (aBrowser.remoteType == E10SUtils.LARGE_ALLOCATION_REMOTE_TYPE) { > + preferredRemoteType = E10SUtils.DEFAULT_REMOTE_TYPE; > + } > + aOptions.remoteType = > + E10SUtils.getRemoteTypeForURI(aURL, What I don't understand is why you don't just pass in E10SUtils.LARGE_ALLOCATION_REMOTE_TYPE to `getRemoteTypeForURI` when `aOptions.freshProcess` is `true`? Is the chance too high that it'll return an invalid remoteType?

Updated in response to mikedeboer's comments, and now redirecting review to olli. MozReview-Commit-ID: LfavqjMcZUq

In bug 1147911, the concept of a remoteType was added to a xul:browser. This was an attribute which would control the type of remote process was intended to be used to load the page. In order to swap two frameLoaders, it has always been necessary for them to either both contain remote content, or both contain non-remote content. This check is made in nsFrameLoader::SwapWithOtherLoader, by checking `IsRemoteFrame() != aOther->IsRemoteFrame()`, and then returining `NS_ERROR_NOT_IMPLEMENTED` if that was not the case. In the follow-up bug 1317293, the check which is being removed here was added to ensure that the remoteType of two frameLoaders which are being swapped also matched. This was not a technical limitation, but rather something which "seemed to make sense to do". This bug removes that check as it is not a technical limitation and causes problems in edge cases around Large-Allocation processes now that the remoteType is being used to denote a Large-Allocation process. Namely, it means that attempting to drag a Large-Allocation window into a new window when at the Large-Allocation process cap will fail, due to being unable to create a new Large-Allocation process in the new window to swap with. The new swapping of the attributes which is added below is done with the intent that the `remoteType` attribute of the xul:browser element should match the `remoteType` attribute of the frameLoader inside of it at all times. As the swap can now occur between two different `remoteType`s, this is necessary to keep that constraint. MozReview-Commit-ID: BHFgjwRGrge

Comment on attachment 8837680 [details] [diff] [review] Part 3: Relax the SwapWithOtherRemoteLoader swap check > returining `NS_ERROR_NOT_IMPLEMENTED` if that was not the case. "returning". Thank you for the explanation. I guess the problem I have with just removing the check is that swapping two different remoteTypes _seems_ pretty weird to me and probably indicates someone screwing up. For the specific case of inter-window drag, how about we introduce a new remoteType value which means "ok to swap anything remote with this, we're about to kill it off anyway", then make the front-end code use that remote type for the inter-window drag case and check for that value in this code?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14) > Thank you for the explanation. I guess the problem I have with just > removing the check is that swapping two different remoteTypes _seems_ pretty > weird to me and probably indicates someone screwing up. I would tend to disagree with that statement. The situations in which people are swapping browsers right now, as far as I know are: * Swapping into a new browser window - in this situation we don't care if the remoteTypes match, because we're going to be destroying one of them anyways. * Swapping into responsive design mode - in this situation we don't care if the remoteTypes match because the window we're swapping with is going to be hidden, and acts pretty much exclusively as a dummy target. * Swapping for GroupedSHistory navigations [currently preffed off by default] - in this situation we want to be able to swap frameloaders which have different remoteTypes, as we would like to be able to swap a frameLoader which was used to load a file:// URI in the `file` remoteType, and a frameLoader which was used to load a https:// URI in the `web` remote type to get seamless bfcache cross process type loading. Ideally in this situation we would want to swap remote and non-remote frameloaders as well, but that isn't supported for technical reasons. Generally, as far as I can tell, we either don't care what the remoteType of the target is, or expressly would like to be able to swap frameLoaders with different remoteTypes. I don't think that it indicates someone "screwing up".

Hmm. OK, I can live with that, I guess.

Comment on attachment 8837680 [details] [diff] [review] Part 3: Relax the SwapWithOtherRemoteLoader swap check r=me modulo the typo fix. It might be nice to have a helper for the "get the remote type" thing, with the fallback for not having the attr set, etc...

Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d242fcdfb7de Part 1: Use remoteType to propagate Large-Allocation status, r=mikedeboer https://hg.mozilla.org/integration/mozilla-inbound/rev/1e634483cd57 Part 2: Simplify the in large allocation process logic, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/60ef04ca2bba Part 3: Relax the SwapWithOtherRemoteLoader swap check, r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/a45659a80a73 Part 4: Add tests for dragging browsers into new windows, r=smaug

https://hg.mozilla.org/mozilla-central/rev/d242fcdfb7de https://hg.mozilla.org/mozilla-central/rev/1e634483cd57 https://hg.mozilla.org/mozilla-central/rev/60ef04ca2bba https://hg.mozilla.org/mozilla-central/rev/a45659a80a73