1338241 - Dragging a Large-Allocation tab into a new window doesn't work

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Use remoteType to propagate Large-Allocation status

MozReview-Commit-ID: LfavqjMcZUq

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Simplify the in large allocation process logic

MozReview-Commit-ID: 6aF2wlLgy42

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Relax the SwapWithOtherRemoteLoader swap check

MozReview-Commit-ID: BHFgjwRGrge

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Add tests for dragging browsers into new windows

MozReview-Commit-ID: XwwrAbeyhy

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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 7

[Olli Pettay [:smaug][[e-mail]]]

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

remove debugger

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Relax the SwapWithOtherRemoteLoader swap check

MozReview-Commit-ID: BHFgjwRGrge

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Add tests for dragging browsers into new windows

MozReview-Commit-ID: XwwrAbeyhy

Comment 11

[Mike de Boer [:mikedeboer]]

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?

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Use remoteType to propagate Large-Allocation status

Updated in response to mikedeboer's comments, and now redirecting review to olli.

MozReview-Commit-ID: LfavqjMcZUq

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Relax the SwapWithOtherRemoteLoader swap check

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 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

[Nika Layzell [:nika] (ni? for response)]

(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".

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

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...

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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