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

RESOLVED FIXED in Firefox 51

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

unspecified
mozilla51
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [multiviewport][reserve-rdm])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

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 hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d84b8c00d76
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)
(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.
Iteration: --- → 51.3 - Sep 12
Flags: qe-verify?
Whiteboard: [multiviewport] → [multiviewport][reserve-rdm]
(Assignee)

Updated

a year ago
Flags: qe-verify? → qe-verify-

Comment 6

a year ago
mozreview-review
Comment on attachment 8789575 [details]
Bug 1301514 - Clarify ownership of BrowserElementIsReady.

https://reviewboard.mozilla.org/r/77740/#review76074
Attachment #8789575 - Flags: review?(kchen) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8789576 [details]
Bug 1301514 - Destroy browser API frame scripts during swap.

https://reviewboard.mozilla.org/r/77742/#review76090
Attachment #8789576 - Flags: review?(kchen) → review+

Comment 8

a year ago
Pushed by jryans@gmail.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/30a7de6065d7
https://hg.mozilla.org/mozilla-central/rev/5947f7b762ef
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.