1301514 - Destroy browser element frame scripts when swapping content out of <iframe mozbrowser>

Status: RESOLVED FIXED

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

Description

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

In the new Responsive Design Mode the DevTools team is working on, we make use of <iframe mozbrowser> to display content.  When you open RDM, we swap content from <xul:browser> to <iframe mozbrowser>.  When you close RDM, we swap content from <iframe mozbrowser> to <xul:browser>.  This all works correctly.

However, a side effect of using <iframe mozbrowser> for a while and then swapping content out of it is that the frame scripts that support the browser element API (BrowserElementChild.js and friends) remain active and listening to various events after the content leaves the browser element.

In most cases, this is only an efficiency loss from running many listeners that are no longer needed.  However, there are also a few cases (such as window.alert handling) where the frame scripts prevent content from responding the way <xul:browser> expects a regular Firefox tab to behave.

To resolve this, let's shutdown the various frame script listeners when swapping out of <iframe mozbrowser>.

Comment 1

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

Attachment: Bug 1301514 - Clarify ownership of BrowserElementIsReady.

It wasn't immediately obvious to me that BrowserElementIsReady was correctly
guarding against re-running the browser element scripts in a frame.  After more
testing, it was working, but I've added some debug lines for future clarity.
No functionality changes in this patch.

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

Comment 2

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

Attachment: Bug 1301514 - Destroy browser API frame scripts during swap.

When swapping content from <iframe mozbrowser> to <xul:browser>, we now stop the
frame scripts that implement the content side of the browser API since they are
no longer needed and can cause issues if they remain active.

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

Comment 3

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

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

Comment 4

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

Curious, what do you mean with "swapping content out"?
I guess swapping the frameloaders?
Why can't you just kill the frame scripts by removing the element from document, or is that not happening? (if that is not happening, it is certainly a bug)

Comment 5

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

(In reply to Olli Pettay [:smaug] from comment #4)
> Curious, what do you mean with "swapping content out"?
> I guess swapping the frameloaders?

Right, swapFrameLoaders is what I mean here.  On closing RDM, we use swapFrameLoaders to move the page content from <iframe mozbrowser> to <xul:browser>.  We use this API so that the state of the content is preserved during the transition, no reloads are needed, etc.

> Why can't you just kill the frame scripts by removing the element from
> document, or is that not happening? (if that is not happening, it is
> certainly a bug)

Removing the <iframe mozbrowser> should indeed stop the frame scripts.  AFAIK, that works correctly.  We don't want to do that in this case since we would also lose the current state of the content (in addition to removing frame scripts) if the page was still in the <iframe mozbrowser> when it was removed.  We call swapFrameLoaders first to move the content out of <iframe mozbrowser> and then remove the <iframe mozbrowser> element.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8789575 [details]
Bug 1301514 - Clarify ownership of BrowserElementIsReady.

https://reviewboard.mozilla.org/r/77740/#review76074

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8789576 [details]
Bug 1301514 - Destroy browser API frame scripts during swap.

https://reviewboard.mozilla.org/r/77742/#review76090

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30a7de6065d7
Clarify ownership of BrowserElementIsReady. r=kanru
https://hg.mozilla.org/integration/autoland/rev/5947f7b762ef
Destroy browser API frame scripts during swap. r=kanru

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30a7de6065d7
https://hg.mozilla.org/mozilla-central/rev/5947f7b762ef