In bug 1238160, we are planning a way to enable <iframe mozbrowser> on desktop. A related feature that is currently implemented only for <xul:browser> is swapFrameLoaders to move content from one browser to another. This should be implemented for <iframe mozbrowser> as well. Of course, it would only be accessible to chrome-scoped JS.

Adds swapFrameLoaders for HTML frames. It is also possible to swap frame loaders between XUL and HTML frames. Review commit: https://reviewboard.mozilla.org/r/39519/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39519/

I'm not likely to get to this before Tuesday.

I'm really sorry for the lag here... This needs pretty careful review and I just failed to make sufficient time for it this week. :( I will aim to get this done on Monday.

Gentle review ping! I agree it's quite a complex area, let me know if there's anything I can do to ease the review.

Sorry this is being so laggy. I had to do a lot of my reviews offline in the last few days, and mozreview turns out to be totally unusable offline. :( Working on this right now.

Comment on attachment 8729622 [details] MozReview Request: Bug 1242644 - HTML swapFrameLoaders. r=bz https://reviewboard.mozilla.org/r/39519/#review38753 ::: dom/base/nsFrameLoader.h:129 (Diff revision 1) > nsresult CloneForStatic(nsIFrameLoader* aOriginal); > > // The guts of an nsIFrameLoaderOwner::SwapFrameLoader implementation. A > // frame loader owner needs to call this, and pass in the two references to > // nsRefPtrs for frame loaders that need to be swapped. > - nsresult SwapWithOtherLoader(nsFrameLoader* aOther, > + nsresult SwapWithOtherLoader(nsIFrameLoaderOwner* aOurLoaderOwner, Why this change? I don't think we should make this change, or add this setter in general. We do NOT want people randomly changing the frameloader on a frameloaderowner! Or is this needed to support swapping between XUL and non-XUL? Do we actually need that? If not, I would rather not support it; it sounds pretty likely to go awry in subtle ways. If we _do_ need this, we need to think really carefully about how it will actually work and whether it's safe in practice. ::: dom/base/nsObjectLoadingContent.cpp:1221 (Diff revision 1) > RefPtr<nsFrameLoader> loader = mFrameLoader; > return loader.forget(); > } > > +NS_IMETHODIMP_(void) > +nsObjectLoadingContent::SetFrameLoader(nsFrameLoader *aFrameLoader) Yeah, this is the sort of footgun I'm thinking about. There is never a good reason to call SetFrameLoader() on an nsObjectLoadingContent. ;) ::: dom/base/test/chrome/window_swapFrameLoaders.xul:104 (Diff revision 1) > + }`; > + } > + > + // Load frame script into each frame > + { > + let frameLoaderOwnerA = frameA.QueryInterface(Ci.nsIFrameLoaderOwner); Why do you need this QI? You shouldn't need to. This isn't XPCOM, and Web IDL bindings don't randomly define more properties when you QI. ;) ::: dom/base/test/chrome/window_swapFrameLoaders.xul:116 (Diff revision 1) > + mmB.loadFrameScript("data:,(" + frameScriptFactory("B") + ")()", false); > + } > + > + // Ping before swap > + { > + let frameLoaderOwnerA = frameA.QueryInterface(Ci.nsIFrameLoaderOwner); Also don't need the QI here. ::: dom/webidl/XULElement.webidl:123 (Diff revision 1) > > // And the things from nsIFrameLoaderOwner > [NoInterfaceObject] > -interface MozFrameLoaderOwner { > +interface MozFrameLoaderOwner {}; > + > +partial interface MozFrameLoaderOwner { Why? This partial interface doesn't make sense to me. Just have it be one interface. ::: dom/xul/nsXULElement.cpp:1670 (Diff revision 1) > - if (&aOtherElement == this) { > - // nothing to do > - return; > - } > + RefPtr<nsFrameLoader> ourLoader = GetFrameLoader(); > + nsCOMPtr<nsIFrameLoaderOwner> ourLoaderOwner = > + do_QueryInterface(NS_ISUPPORTS_CAST(nsIDOMXULElement*, this)); > + nsCOMPtr<nsIFrameLoaderOwner> otherLoaderOwner = &aOtherOwner; > > - nsXULSlots *ourSlots = static_cast<nsXULSlots*>(GetExistingDOMSlots()); > + if (!ourLoader) { This lost the check on aOtherElement's frame loader existing. And the callee (SwapWithOtherLoader) doesn't do that check either.

Oh, one other thing. We probably shouldn't allow swapping between an isolated and a non-isolated mozbrowser, right?

https://reviewboard.mozilla.org/r/39519/#review38753 > Why this change? I don't think we should make this change, or add this setter in general. We do NOT want people randomly changing the frameloader on a frameloaderowner! > > Or is this needed to support swapping between XUL and non-XUL? Do we actually need that? If not, I would rather not support it; it sounds pretty likely to go awry in subtle ways. > > If we _do_ need this, we need to think really carefully about how it will actually work and whether it's safe in practice. It is related to swapping between XUL and HTML, which I do actually want to support. I plan to use it from responsive design mode in DevTools to adopt content from a XUL browser into an HTML browser. (So far it appears to work well!) So, let's think about how to make it work safely. My C++ is not the best, so there may be an easier way. I tried to keep the original API signature at first: nsresult SwapWithOtherLoader(nsFrameLoader* aOther, RefPtr<nsFrameLoader>& aFirstToSwap, RefPtr<nsFrameLoader>& aSecondToSwap); but it seemed like the `swap` call within `SwapWithOtherLoader` would only swap **copies** of the RefPtrs in each nsIFrameLoaderOwner. This may have been because I had to first pull the RefPtr from nsIFrameLoaderOwner, since it's stored in different ways per concrete type (XUL vs. HTML). So, that's how I arrived at the current design which adds SetFrameLoader: so I can update the pointers to frame loaders in each owner. It might be possible to simplify the new API to just: nsresult SwapWithOtherLoader(nsIFrameLoaderOwner* aOtherLoaderOwner); since I think the nsFrameLoader instance can reach it's own owner by QIing mOwnerContent to nsIFrameLoaderOwner. Is there a way to capture the "right" instances of RefPtr (not copies) so the old API can continue, or should I proceed with the current path? > Yeah, this is the sort of footgun I'm thinking about. There is never a good reason to call SetFrameLoader() on an nsObjectLoadingContent. ;) I agree, we don't need SetFrameLoader to work here. It's just here since the class implements nsIFrameLoaderOwner. I could change it to return an nsresult with a failure code here? > Why do you need this QI? You shouldn't need to. This isn't XPCOM, and Web IDL bindings don't randomly define more properties when you QI. ;) Okay, will remove. I think I borrowed this snippet from other code that does this. > Why? This partial interface doesn't make sense to me. Just have it be one interface. It's a partial interface because of the circular reference: swapFrameLoaders has a parameter that is the same interface as the one we are currently defining. The WebIDL parser does not seem to accept the circular reference in the main / single interface body. > This lost the check on aOtherElement's frame loader existing. And the callee (SwapWithOtherLoader) doesn't do that check either. I'll bring that back in, thanks!

(In reply to Boris Zbarsky [:bz] from comment #8) > Oh, one other thing. We probably shouldn't allow swapping between an > isolated and a non-isolated mozbrowser, right? Right, that case seems covered already I think: https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/dom/base/nsFrameLoader.cpp#1203 It does not seems covered at the moment in the SwapWithOtherRemoteLoader path. I'll have to see if we have enough info to do the check there.

Please look over my replies in comment 9 and comment 10.

> It is related to swapping between XUL and HTML, which I do actually want to support So here is the problem as far as I'm concerned... It's not the mechanics of making the swap work that worry me; that part is solvable. What worries me is that swapping between two elements should lead to the same state as doing the loads in those elements to start with. This is why we have the various "make sure they're the same docshell type" bits and whatnot. So we need to make sure that whatever state we end up with after the swap is basically the same state as we'd end up in if we just did |this.src = this.contentWindow.location| on both elements involved after performing the swap (ignoring for the moment that this would lose DOM state and whatnot). If we really need to support this then we need to very carefully audit what state loads in these various elements take from their element and that swapping them is not safe. For example, note my comments above about isolated mozbrowsers. Clearly at best you can only swap isolated or non-isolated mozbrowsers with a <xul:browser>, since isolated and non-isolated mozbrowsers should never be swapped with each other. Which one of them should be swappable with a <xul:browser> and why? > It's a partial interface because of the circular reference: swapFrameLoaders has a parameter that is the same interface as > the one we are currently defining. The WebIDL parser does not seem to accept the circular reference in the main / single > interface body. Circular references should be ok in IDL; they're all over Node.webidl, right? ;) The real problem you have is that you're trying to define the same identifier to be both a mixin and an external interface. Then the binding machinery gets confused because it assumes those things are mutually exclusive (it should probably error out instead, really). You just want to use different names for the mixin and the argument type. Again, assuming that we end up allowing nsIFrameLoaderOwner as the argument type here at all.

OK, so per that it looks like we want to allow swapping non-isolated mozbrowser with xul:browser?

(In reply to Boris Zbarsky [:bz] from comment #13) > OK, so per that it looks like we want to allow swapping non-isolated > mozbrowser with xul:browser? Yes, that's right, we should only allow swapping non-isolated browsers with xul:browsers, since these are the two browser variants that share access to cookies, etc. while isolated mozbrowser does not. This is already what happens with the existing check I linked to: GetIsIsolatedMozBrowserElement() is false for all xul:browsers and GetIsIsolatedMozBrowserElement() is false for non-isolated mozbrowser. For an isolated mozbrowser, the values would not match, so the swap is correctly blocked. So effectively it only allows XUL browsers and non-isolated mozbrowsers to swap. It does seem like we should carry over a similar check to the SwapWithOtherRemoteLoader path if we can.

OK. What other things about the document inside the element are controlled by element attributes or the namespace/localName of the element? Are we already checking all of them?

(In reply to Boris Zbarsky [:bz] from comment #15) > OK. What other things about the document inside the element are controlled > by element attributes or the namespace/localName of the element? Are we > already checking all of them? Here's my audit of features triggered by different attributes / element namespaces: # Attributes / Namespaces with impact on frame features * @remote We already verify both frame loaders have matching remoteness. https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/dom/base/nsFrameLoader.cpp#1058 * GetIsIsolatedMozBrowserElement() As stated earlier in this bug, we do check this for the non-remote case, but may need to add a check for the remote case. * @userContextId (XUL only) We should add checks to verify that the userContextId matches. Since only XUL elements currently support userContextId, mozbrowser frames would only be allowed to swap with xul:browsers using the "default" user content (no ID supplied). * docshell type Various aspects of docshell types are verified in the non-remote case. Remote browsers are only created for content docshells AFAIK, so we can assume the type matches in the remote case. https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/dom/base/nsFrameLoader.cpp#1090-1150 * @allowfullscreen (mozbrowser only) With mozbrowser frames, you need to set @allowfullscreen on frames to allow fullscreen behavior for child frames. With XUL frames, fullscreen appears to always be allowed. We should add a check to verify the frames have equivalent fullscreen behavior before swapping, so mozbrowser frames would need to have allowfullscreen before swapping with a XUL frame. * @srcdoc (HTML only) HTML frames support srcdoc while XUL does not. We should add a check to verify no HTML frame with srcdoc tries to swap into a XUL frame. * @disablehistory (XUL only) XUL frames allow session history to be disabled, while it is always enabled for HTML frames. We do check this in the non-remote case, but there seems to be no check for the remote case, so we should try to add something there. https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/dom/base/nsFrameLoader.cpp#1102-1110 * @mozpasspointerevents Frames of any type will pass through pointer events if @mozpasspointerevents is set. We should add a check to ensure both frames are in the same state for this attribute. # Summary Overall, there are several checks we need to add, and some are for XUL-only features (so it would have been more correct for them to already be in place). Others are needed because of the expansion to XUL <-> HTML swapping in this bug or were only checked in non-remote path. I'll add the missing checks for the next version of the patch here.

Thank you for going through that! I really appreciate it. It's certainly going to make me worry a lot less about doing the XUL to HTML swapping. :)

Comment on attachment 8729622 [details] MozReview Request: Bug 1242644 - HTML swapFrameLoaders. r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39519/diff/1-2/

Comment on attachment 8729622 [details] MozReview Request: Bug 1242644 - HTML swapFrameLoaders. r=bz https://reviewboard.mozilla.org/r/39519/#review39557 ::: dom/base/nsFrameLoader.cpp:920 (Diff revisions 1 - 2) > + other->mRemoteBrowser->IsIsolatedMozBrowserElement() || > + mRemoteBrowser->HasOwnApp() != other->mRemoteBrowser->HasOwnApp()) { > + return NS_ERROR_NOT_IMPLEMENTED; > + } > + > + if (mRemoteBrowser->OriginAttributesRef().mUserContextId != Shouldn't this just check all the origin attributes? I don't think we want to swap things with different origin attributes at all, right? ::: dom/base/nsFrameLoader.cpp:1313 (Diff revisions 1 - 2) > + otherDocshell->GetFullscreenAllowed(&otherFullscreenAllowed); > + if (ourFullscreenAllowed != otherFullscreenAllowed) { > + return NS_ERROR_NOT_IMPLEMENTED; > + } > + > + bool ourHasSrcdoc = ourContent->IsHTMLElement(nsGkAtoms::iframe) && Some of these checks are duplicated between SwapWithOtherLoader and SwapWithOtherRemoteLoader. Can you please just put them in SwapWithOtherLoader _before_ it calls out to SwapWithOtherRemoteLoader? Seems like that would reduce the code duplication. ::: dom/webidl/XULElement.webidl:130 (Diff revision 2) > > [ChromeOnly] > void setIsPrerendered(); > > [ChromeOnly, Throws] > - void swapFrameLoaders(XULElement aOtherOwner); > + void swapFrameLoaders(nsIFrameLoaderOwner aOtherOwner); OK, so I think this part should work as follows: 1) Add overloads of swapFrameLoaders taking XULElement and HTMLIFrameElement. 2) Add public SwapFrameLoaders functions on the C++ HTMLIFrameElement and XULElement that take a RefPtr<FrameLoader>&. 3) When invoking the overload that takes the same type as `this`, we can just call the existing SwapWithOtherLoader directly. The XUL code for this would stay as-is; the HTML code would need to be added. 4) When invoking the overload that takes the other type (XULElement when `this` is HTMLIFrameElement, say), call SwapFrameLoader on the argument and pass it the reference to our frame loader's storage. It can then call nsFrameLoader::SwapWithOtherLoader as needed, because it can dig up its own reference. The idea here is to prevent having an explicit frame loader setter on nsIFrameLoaderOwner, which I really would much rather avoid.

https://reviewboard.mozilla.org/r/39519/#review39557 > Shouldn't this just check all the origin attributes? I don't think we want to swap things with different origin attributes at all, right? Makes sense, I've made this change. > Some of these checks are duplicated between SwapWithOtherLoader and SwapWithOtherRemoteLoader. Can you please just put them in SwapWithOtherLoader _before_ it calls out to SwapWithOtherRemoteLoader? Seems like that would reduce the code duplication. Okay, I've done this for the checks that used the same approaches to access data in both cases. A few of the checks used different approaches for remote vs. non-remote, so I've left those where there are. > OK, so I think this part should work as follows: > > 1) Add overloads of swapFrameLoaders taking XULElement and HTMLIFrameElement. > > 2) Add public SwapFrameLoaders functions on the C++ HTMLIFrameElement and XULElement that take a RefPtr<FrameLoader>&. > > 3) When invoking the overload that takes the same type as `this`, we can just call the existing SwapWithOtherLoader directly. The XUL code for this would stay as-is; the HTML code would need to be added. > > 4) When invoking the overload that takes the other type (XULElement when `this` is HTMLIFrameElement, say), call SwapFrameLoader on the argument and pass it the reference to our frame loader's storage. It can then call nsFrameLoader::SwapWithOtherLoader as needed, because it can dig up its own reference. > > The idea here is to prevent having an explicit frame loader setter on nsIFrameLoaderOwner, which I really would much rather avoid. Thanks, this appears to be working well! :) I have also removed the XPCOM (.idl) swapFrameLoaders method in this round, as it seems to be redundant now that the WebIDL version exists.

Comment on attachment 8729622 [details] MozReview Request: Bug 1242644 - HTML swapFrameLoaders. r=bz Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39519/diff/2-3/

Comment on attachment 8729622 [details] MozReview Request: Bug 1242644 - HTML swapFrameLoaders. r=bz https://reviewboard.mozilla.org/r/39519/#review39689 ::: dom/base/nsFrameLoader.cpp:1128 (Diff revision 3) > + if (ourPassPointerEvents != otherPassPointerEvents) { > + return NS_ERROR_NOT_IMPLEMENTED; > + } > + > + // Divert to a separate path for the remaining steps in the remote case > + if (IsRemoteFrame() && aOther->IsRemoteFrame()) { Given that we tested earlier that IsRemoteFrame() == aOther->IsRemoteFrame(), not much point in testing both here... Could test one and assert the other? ::: dom/base/nsFrameLoader.cpp:1270 (Diff revision 3) > otherDocshell->GetIsIsolatedMozBrowserElement() || > ourDocshell->GetIsApp() != otherDocshell->GetIsApp()) { > - return NS_ERROR_NOT_IMPLEMENTED; > + return NS_ERROR_NOT_IMPLEMENTED; > } > > + if (ourDocshell->GetOriginAttributes() != We should make sure to test dragging a non-default user context tab out of windows and between windows, by the way. Just manual testing is fine. ::: dom/html/nsGenericHTMLFrameElement.cpp:225 (Diff revision 3) > + if (&aOtherLoaderOwner == this) { > + // nothing to do > + return; > + } > + > + if (!aOtherLoaderOwner.mFrameLoader) { Why bother checking this? The other overload of SwapFrameLoaders you're calling will check it anyway. r=me. Thank you again for checking through all the edge cases here!

https://reviewboard.mozilla.org/r/39519/#review39689 > Given that we tested earlier that IsRemoteFrame() == aOther->IsRemoteFrame(), not much point in testing both here... Could test one and assert the other? Okay, I've moved the check for the other frame's remoteness to an assert. > We should make sure to test dragging a non-default user context tab out of windows and between windows, by the way. Just manual testing is fine. Actually it does break moving tabs for non-default user context IDs. Since the user context feature is currently disabled by default, I'll continue with landing this check as-is. I filed bug 1260766 about the problem. > Why bother checking this? The other overload of SwapFrameLoaders you're calling will check it anyway. Okay, I'll remove these redundant checks.