1268688 - Start browser element API when swapFrameLoaders moves from XUL to HTML frame

Status: RESOLVED FIXED

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

Description

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

In bug 1242644, we added support for swapFrameLoaders with <iframe mozbrowser>, including across frame types (XUL <-> HTML).

One edge case that comes up is:

1. Create XUL frame A
2. Create HTML frame B
3. swapFrameLoaders
4. Try to use APIs from BrowserElement.webidl

Currently the APIs fail because:

* BrowserElementParent.js is listening on the old message manager and needs to switch to the new one
* The frame script BrowserElementChild.js has not been loaded for the frame that moved into HTML

Comment 1

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

Attachment: MozReview Request: Bug 1268688 - Start browser API for frames swapping to HTML. r=bz

Review commit: https://reviewboard.mozilla.org/r/49567/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49567/

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4107f9064be

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8746782 [details]
MozReview Request: Bug 1268688 - Start browser API for frames swapping to HTML. r=bz

https://reviewboard.mozilla.org/r/49567/#review46559

::: dom/base/nsFrameLoader.cpp:1006
(Diff revision 1)
>    otherShell->BackingScaleFactorChanged();
>  
>    ourDoc->FlushPendingNotifications(Flush_Layout);
>    otherDoc->FlushPendingNotifications(Flush_Layout);
>  
> +  // Initialize browser API if needed now that owner content has changed.

This won't "tear down" the browser API in the place where it should no longer exist, right?  Should it?

::: dom/base/nsFrameLoader.cpp:1014
(Diff revision 1)
> +
>    mInSwap = aOther->mInSwap = false;
>  
> -  Unused << mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
> -  Unused << aOther->mRemoteBrowser->SendSwappedWithOtherRemoteLoader();
> +  // Send an updated tab context since owner content type may have changed.
> +  MutableTabContext ourContext;
> +  rv = GetNewTabContext(&ourContext);

This will ignore the signed package stuff, if any.  Is that OK?

r=me

Comment 4

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

https://reviewboard.mozilla.org/r/49567/#review46559

> This won't "tear down" the browser API in the place where it should no longer exist, right?  Should it?

That's correct, for a frame that moves from HTML -> XUL, the browser API is still running, even though it can't be accessed since the API is not exposed for such elements.  I think that's okay for now, but if we find that it causes problems, we can add an explicit tear down step in follow up work.

I also noticed that since the child frame script side is always loaded, it may repeat its setup unnecessarily (in the HTML -> HTML swap case, for example).  I'll add a guard to make sure the child frame script aborts early if its work is already done.

> This will ignore the signed package stuff, if any.  Is that OK?

Yes, that seems okay to me.  I am not aware of a current use case for swapFrameLoaders where signed packages are also used.  If someone tries to use it, we'll currently crash the child, since the new tab context would have missing / different signed package info.  We can address this in the future if it comes up.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88d9ca956c2c

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/88d9ca956c2c

Comment 7

[Tanvi Vyas[:tanvi]]

This is causing a crash as described here: https://bugzilla.mozilla.org/show_bug.cgi?id=1271792

Comment 8

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

(In reply to Tanvi Vyas [:tanvi] from comment #7)
> This is causing a crash as described here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1271792

I'll reply in the new bug.