Closed Bug 1058164 Opened 5 years ago Closed 5 years ago

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

Categories

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

31 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
That all is for non-e10s of course.
In case of e10s we should probably dispatch events to xul:browsers _and_ to TabChildGlobals
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.
Depends on: 918634
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).
Attachment #8480198 - Flags: feedback?(bugs)
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.
Attachment #8480198 - Flags: feedback?(bugs) → feedback+
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?
Flags: needinfo?(bugs)
Ah, I wasn't looking that closely when giving the previous feedback.

Yes, sounds right. (this needs some tests)
Flags: needinfo?(bugs)
Sounds good. Tests coming up.
Summary: pageshow and pagehide events are not dispatched to content scripts when swapping frame loaders → pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders
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.
Attachment #8480198 - Attachment is obsolete: true
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.
Attachment #8480978 - Flags: review?(felipc)
Attachment #8480978 - Flags: review?(bugs)
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!
Attachment #8480979 - Flags: review?(bugs)
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 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.
Attachment #8480978 - Flags: review?(bugs) → review+
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.
Attachment #8480979 - Flags: review?(bugs) → review-
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.
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.
Attachment #8482808 - Attachment is obsolete: true
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.
Attachment #8482816 - Attachment description: pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=? → pageshow and pagehide events with the persisted property are not dispatched to content scripts when swapping frame loaders. r=smaug
Attachment #8482816 - Flags: review?(felipc)
Attachment #8480978 - Attachment is obsolete: true
Attachment #8480978 - Flags: review?(felipc)
Attachment #8480979 - Attachment is obsolete: true
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!
Attachment #8482806 - Flags: review?(bugs)
Attachment #8482816 - Flags: review?(felipc) → review+
Thanks! Here's a try push for good measure... https://tbpl.mozilla.org/?tree=Try&rev=09f56c51c8cd
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.
Attachment #8482806 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/e92f2cc211ca
https://hg.mozilla.org/mozilla-central/rev/c537b894b7f4
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla35
Blocks: 1001699
Blocks: 1063983
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.