1136478 - [e10s] pagehide/show events not fired when swapping remote browsers

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

+++ This bug was initially created as a clone of Bug #1084637 +++

Basically, we're not firing these events in SwapWithOtherRemoteLoader. I'm not sure what the consequences of this are. Olli, is it important for content to be able to get these events? Or is it just something we do for chrome code?

Comment 1

[Boris Zbarsky [:bzbarsky]]

nsFrameLoader::SwapWithOtherLoader only fires these events on the chrome event handler for the window.  Note this comment:

1221   // Fire pageshow events on still-loading pages, and then fire pagehide
1222   // events.  Note that we do NOT fire these in the normal way, but just fire
1223   // them on the chrome event handlers.

So content shouldn't be getting these events no matter what.  They're there so extensions listening for them on the <tabbrowser> won't get confused by getting pagehide without a corresponding pageshow or vice versa.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

OK, thanks. Should have read more carefully. This doesn't seem like a very high priority then.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Note also that browser_bug1058164.js is disabled for e10s, ostensibly due to bug 918634, but in reality it is due to this bug (fails with "Timed out waiting for pagehide")

Comment 4

[Jim Mathies [:jimm]]

This blocks an M7.

Comment 5

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

Attachment: MozReview Request: bz://1136478/mconley

/r/8353 - Bug 1136478 - Fire pagehide / pageshow events in content after swapping remote frame loaders. r=?

Pull down this commit:

hg pull -r 65401282388bcd42dd876aeb1ea463c19f661a7a https://reviewboard-hg.mozilla.org/gecko/

Comment 6

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

Comment on attachment 8602786 [details]
MozReview Request: bz://1136478/mconley

/r/8353 - Bug 1136478 - Fire pagehide / pageshow events in content after swapping remote frame loaders. r=?

Pull down this commit:

hg pull -r 65401282388bcd42dd876aeb1ea463c19f661a7a https://reviewboard-hg.mozilla.org/gecko/

Comment 7

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

I am planning on writing a regression test for this, but I just wanted to know from smaug whether or not my approach looks sane.

Comment 8

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

Actually, I think the browser_CTP_drag_drop.js test covers this - at least, it covers something that's dependent.

smaug - if you think it's worth it, I'll write a more specific mochitest for these events. Otherwise, I'll just rely on browser_CTP_drag_drop.js.

Comment 9

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

Comment on attachment 8602786 [details]
MozReview Request: bz://1136478/mconley

I don't think we can really do that two async messages thing.
Anything can happen on content side between those.
Can you somehow merge the messages to one, so that we dispatch pagehide/show
synchronously on child side?

Comment 10

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

Comment on attachment 8602786 [details]
MozReview Request: bz://1136478/mconley

/r/8353 - Bug 1136478 - Fire pagehide / pageshow events in content after swapping remote frame loaders. r=?

Pull down this commit:

hg pull -r eaeb8990e99612b9dcbb17fd7488521d4f441d6f https://reviewboard-hg.mozilla.org/gecko/

Comment 11

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

I think 
+  unused << mRemoteBrowser->SendSwappingFrameLoaders();
+  unused << aOther->mRemoteBrowser->SendSwappingFrameLoaders();
should be at the end of nsFrameLoader::SwapWithOtherRemoteLoader

And the message name in ipdl could be then SwappedWithOtherRemoteLoader()
(because of async nature of the messaging, the swapping has happened already when child receives the messages)


+TabChild::RecvSwappingFrameLoaders()
+{
+  nsCOMPtr<nsIDocShell> ourDocShell = do_GetInterface(WebNavigation());
+  nsCOMPtr<nsPIDOMWindow> ourWindow = ourDocShell->GetWindow();
ourDocShell is possibly null here.

+  nsCOMPtr<EventTarget> ourEventTarget = ourWindow->GetParentTarget();
in thery ourWindow could be null here.
So, null checks please.

Comment 12

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

Comment on attachment 8602786 [details]
MozReview Request: bz://1136478/mconley

/r/8353 - Bug 1136478 - Fire pagehide / pageshow events in content after swapping remote frame loaders. r=smaug.

Pull down this commit:

hg pull -r fbe5ee7bb8aeac7edabb580bcab0254aebcbc1e0 https://reviewboard-hg.mozilla.org/gecko/

Comment 13

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

Thanks! I moved the messages to the end, changed the name, and added null checks as requested.

Will confirm a green try build and then land.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb58a88c4529

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eb58a88c4529

Comment 17

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

Attachment: MozReview Request: Bug 1136478 - Fire pagehide / pageshow events in content after swapping remote frame loaders. r=smaug.

Comment 18

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

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1136478.html