1058164 - pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders

Status: RESOLVED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

I noticed this while fixing up bug 899347.

When we drag a tab out to a new window, we swap frameloaders with the new window, and the pageshow event gets fired because we're pulling the page out of the bfcache.

Unfortunately, content scripts for the frame loader don't seem to hear the pageshow event. A look at nsFrameLoader.cpp shows that we're dispatching those events to the chrome event handler, which bypasses content scripts on the frameloader.

smaug suggested that we dispatch the events to the parent targets of each window instead.

Presumably, this bug applies to pagehide as well, so both should be fixed in tandem.

Comment 1

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

That all is for non-e10s of course.
In case of e10s we should probably dispatch events to xul:browsers _and_ to TabChildGlobals

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Pretty sure we'll need bug 918634 before we fix this for e10s - or, at the very least, we'll need to coordinate with handyman, who's working on that bug.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP: pageshow and pagehide events are not dispatched to content scripts when swapping frame loaders. r=?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8480198 [details] [diff] [review]
WIP: pageshow and pagehide events are not dispatched to content scripts when swapping frame loaders. r=?

Hey smaug, is this the right idea for the nsFrameLoader bits? (for the non-e10s case).

Comment 5

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

Comment on attachment 8480198 [details] [diff] [review]
WIP: pageshow and pagehide events are not dispatched to content scripts when swapping frame loaders. r=?

For the non-e10s + frameloader yes.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8480198 [details] [diff] [review]
WIP: pageshow and pagehide events are not dispatched to content scripts when swapping frame loaders. r=?

Review of attachment 8480198 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsFrameLoader.cpp
@@ +1347,5 @@
>  
>    ourParentDocument->FlushPendingNotifications(Flush_Layout);
>    otherParentDocument->FlushPendingNotifications(Flush_Layout);
>  
> +  FirePageShowEvent(ourDocshell, otherEventTarget, true);

Hey smaug - this bit right here... am I right in thinking I actually have this reversed?

It makes sense to dispatch to the otherChromeEventHandler for ourDocshell, because we assigned otherChromeEventHandler as the chrome event handler for ourDocshell earlier in the method, and we have these old references to otherChromeEventHandler and ourChromeEventHandler, and so we just pass the opposites in instead of swapping the values of the variables as well...

But for ourEventTarget and otherEventTarget... those things are _moving_ with the frame loaders when they get swapped, right? Shouldn't this be:

FirePageShowEvent(ourDocshell, ourEventTarget, true);
FirePageShowEvent(otherDocshell, otherEventTarget, true);

instead?

Comment 7

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

Ah, I wasn't looking that closely when giving the previous feedback.

Yes, sounds right. (this needs some tests)

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Sounds good. Tests coming up.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=?

Frame scripts were not getting pageshow and pagehide events with the persisted
property during frameloader swapping due to the fact that the event target for
those events was set to be the chrome event handler. This patch makes the
event target be the nsInProcessTabChildGlobal instead, so frame scripts see
the events. This still needs to be fixed for the e10s case, but that will
have to wait until bug 918634 is fixed.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Test that we get pageshow and pagehide events in content scripts when swapping frameloaders. r=?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8480978 [details] [diff] [review]
pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=?

Asking for review from smaug for the nsDocument bits, and felipe for the browser.js bit.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8480979 [details] [diff] [review]
Test that we get pageshow and pagehide events in content scripts when swapping frameloaders. r=?

smaug, are you the right person to review this test? If not, let me know who I should direct it to. Thanks!

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8480978 [details] [diff] [review]
pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=?

Review of attachment 8480978 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ -1031,5 @@
> -      // pageshow handlers are being migrated to
> -      // content.js. Eventually this code should be removed.
> -      gBrowser.addEventListener("pageshow", function(event) {
> -        // Filter out events that are not about the document load we are interested in
> -        if (content && event.target == content.document)

isn't this filtering being lost? Or does it not matter for some reason?

I imagine that the setTimeout is no longer necessary due to the message passing, but I don't see any comments on why it exist, so I suppose we'd have to dig history here to make sure we test that it can be removed.

Comment 14

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

Comment on attachment 8480978 [details] [diff] [review]
pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=?

So this all is still for non-e10s only, right, and e10s requires bug 918634 

>-        if (content && event.target == content.document)
>-          setTimeout(pageShowEventHandlers, 0, event.persisted);
>-      }, true);
As felipe mentioned, you probably don't want to miss this filtering.


r+ for the frameloader.

Comment 15

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

Comment on attachment 8480979 [details] [diff] [review]
Test that we get pageshow and pagehide events in content scripts when swapping frameloaders. r=?

Please don't use Promise.jsm but the native Promise implementation.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Test that we get pageshow and pagehide events in content scripts when swapping frameloaders. r=?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=?

Frame scripts were not getting pageshow and pagehide events with the persisted
property during frameloader swapping due to the fact that the event target for
those events was set to be the chrome event handler. This patch makes the
event target be the nsInProcessTabChildGlobal instead, so frame scripts see
the events. This still needs to be fixed for the e10s case, but that will
have to wait until bug 918634 is fixed.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=smaug

Frame scripts were not getting pageshow and pagehide events with the persisted
property during frameloader swapping due to the fact that the event target for
those events was set to be the chrome event handler. This patch makes the
event target be the nsInProcessTabChildGlobal instead, so frame scripts see
the events. This still needs to be fixed for the e10s case, but that will
have to wait until bug 918634 is fixed.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8482816 [details] [diff] [review]
pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=smaug

Ok, I've re-added the filtering. I had to break it down into 2 parts:

The first part is in the content script, and filters out pageshow events that are not for the top-level document.

The second part is in browser.js and filters out PageVisibility:Show messages that are not from the active tab.

I'm not 100% sure that first part is necessary. I have a feeling that reacting to pageshow events for subdocuments is desirable, but right now I'm just aiming for parity with our current behaviour. I'll file a follow-up bug to investigate listening to pageshow events in subdocuments after this lands.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8482806 [details] [diff] [review]
Test that we get pageshow and pagehide events in content scripts when swapping frameloaders. r=?

DOM Promises it is!

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Thanks! Here's a try push for good measure... https://tbpl.mozilla.org/?tree=Try&rev=09f56c51c8cd

Comment 22

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

Comment on attachment 8482806 [details] [diff] [review]
Test that we get pageshow and pagehide events in content scripts when swapping frameloaders. r=?

>+ * Content script injected in the test browser that sends
I have no idea what "Content script" means in this context.


>+ * messages when it sees the pagehide and pageshow events
>+ * with the persisted property set to true.
>+ */
>+function content_script() {
ah, so this will be serialized.
Anyhow, please use term "frame script"

>+/**
>+ * Returns a Promise that resolves when the load event for
>+ * aWindow fires during the capture phase.
>+ */
Sounds like a crazy Promise when one could just add an event listener.
But fine, if you like adding extra complexity using Promises ;)



>+/**
>+ * Tests that content scripts get pageshow / pagehide events when
>+ * swapping browser frameloaders (which occurs when moving a tab
>+ * into a different window).
>+ */
>+add_task(function* test_swap_frameloader_pagevisibility_events() {
>+  // Inject our content script into a new browser.
>+  let tab = gBrowser.addTab();
>+  gBrowser.selectedTab = tab;
>+  let mm = window.getGroupMessageManager("browsers");
>+  mm.loadFrameScript("data:,(" + content_script.toString() + ")();", true);
>+  let browser = gBrowser.selectedBrowser;
>+
>+  // Swap the browser out to a new window
>+  let newWindow = gBrowser.replaceTabWithWindow(tab);
>+  // We have to wait for the window to load so we can get the selected browser
>+  // to listen to.
>+  yield waitForLoad(newWindow);
>+  let pageHideAndShowPromise = prepareForPageHideAndShow(newWindow.gBrowser.selectedBrowser);
>+  // Yield waiting for the pagehide and pageshow events.
>+  yield pageHideAndShowPromise;
>+
>+  // Send the browser back to its original window
>+  let newTab = gBrowser.addTab();
>+  gBrowser.selectedTab = newTab;
>+  browser = newWindow.gBrowser.selectedBrowser;
>+  pageHideAndShowPromise = prepareForPageHideAndShow(gBrowser.selectedBrowser);
>+  gBrowser.swapBrowsersAndCloseOther(newTab, newWindow.gBrowser.selectedTab);
>+
>+  // Yield waiting for the pagehide and pageshow events.
>+  yield pageHideAndShowPromise;
>+
>+  gBrowser.removeTab(gBrowser.selectedTab);
>+});

mostly guessing this task.js stuff does the right thing.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Thanks! Fixed issues.

remote:   https://hg.mozilla.org/integration/fx-team/rev/e92f2cc211ca
remote:   https://hg.mozilla.org/integration/fx-team/rev/c537b894b7f4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e92f2cc211ca
https://hg.mozilla.org/mozilla-central/rev/c537b894b7f4