364461 - [FIX]Pagehide not always dispatched in subframes

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Sylvain Pasche]

In subframes, I could find situations where the pagehide event is not thrown. The unload event is always sent for subframes in any case, which is not true for pagehide.

I faced this bug while I wanted to convert an extension from using unload handlers to pagehide ones, so that the bfcache is not disabled. One workaround is still to attach pagehide listeners on the main frame, and unload handler for subframes, but  using pagehide in all situations would be more coherent.

The steps for reproducing are in the testcase.

Comment 1

[Sylvain Pasche]

Attachment: testcase subframe

Comment 2

[Sylvain Pasche]

Attachment: main testcase

Comment 3

[Sylvain Pasche]

Attachment: hack which solves the issue (for demonstration)

After playing a bit with gdb, here's my observations:

nsDocShell::FirePageHideNotification is from where the pagehide events are dispatched (obviously).

This function recurses into the child docShells to call the pagehide events on subframes. However, in some cases, the mFiredUnloadEvent in the child docshells is true, so the pagehide events are not called.

The mFiredUnloadEvent property is set to false in nsDocShell::CreateContentViewer or in nsDocShell::RestoreFromHistory.

But in the RestoreFromHistory case, it only resets the mFiredUnloadEvent property on the current docShell, but not the child docShells.

The attached hack resets the mFiredUnloadEvent for child docShells, so the pagehide event are fired for all subframes.

So the next step would be do deal with this in a sane way (that's where understanding the docShell would help ;-) )

Comment 4

[Sylvain Pasche]

Some useful comments from Boris:

22:48 <@bz> this sounds like the right approach
22:48 <@bz> nsDocShell::FirePageHideNotification sets mFiredUnloadEvent
22:48 <@bz> You'll need to recurse, though
22:48 <@bz> not just do it on the kids

22:50 <@bz> syp: perhaps reset it in BeginRestore?
22:51 <@bz> syp: or in FinishRestore?
22:51  * bz suspects BeginRestore is a better spot


I'll have a look at this later and propose a real patch.

Comment 5

[Sylvain Pasche]

Attachment: version 1

Clearing the mFiredUnloadEvent flag in FinishRestore() solves this specific issue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So why FinishRestore and not BeginRestore?

Also, I'm not sure I can review this in a reasonable time frame (like before mid-April).  Try biesi or bryner?

Comment 7

[Sylvain Pasche]

(In reply to comment #6)
> So why FinishRestore and not BeginRestore?

From what I saw, BeginRestore is first called before the docshell has switched from the old to the new document, so setting the flag there does not work as expected (that's more a trial and error than detailed understanding I must admit).

> Also, I'm not sure I can review this in a reasonable time frame (like before
> mid-April).  Try biesi or bryner?

ok fine, I'll ask them.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Well, I ended up debugging this over again due to bug 384977...  Patch that I think is more correct coming up once I've written some testcases.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I started trying to write tests, then realized that it's a lot more work than I'll be able to put in at a stretch any time soon.  We need to test proper firing of pagehide/pageshow with bfcache both on (that is, not disabled by any of the features the testcase uses!) and off, test that pagehide is fired repeatedly on subframes (this bug), and test that loads can happen in a subframe after going back to the frameset (bug 384977).  Help writing the tests would be _much_ appreciated.  mochitest is probably the way to go.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Patch to fix.  Note that I added the chunk in InternalLoad to prevent a frame in unload doing a targeted load in a different frame that ends up getting bounced to itself; that's a case bug 371360 missed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 268917 [details] [diff] [review]
Proposed fix

We need this on branch if we take bug 371360.

Comment 13

[Sylvain Pasche]

I also tried to write a small mochitest for this bug at the time, but mochitest environment could not reproduce the issue. The tests are run in subframes, and the bfcache behaves differently in that situation (bug 304860).

Now that mochitest support chrome based tests, we could run the tests inside a topolevel window containing a <browser>. I can try porting my old testcase in such  an environment. That was simply something like loading a page, moving forward/backward and counting the events (pagehide/pageshow/load, ...) received. What do you think of that approach?

Comment 14

[Boris Zbarsky [:bzbarsky]]

If it reproduces the bug, that sounds great!

Comment 15

[Daniel Veditz [:dveditz]]

wanted on the branch, but not stop-ship (unless we missed something about why it was nominated)

Comment 16

[Sylvain Pasche]

Attachment: unit test

Here's the chrome unit test as proposed. I've added checks with a document without iframes, and one with an unload handler for completeness. The real test for this bug is marked by the comment "This is where the two nested pagehide are not dispatched".

Who should I ask for review on this?

Comment 17

[Jeff Walden [:Waldo]]

I'd suggest bz since he's already familiar with the bug, but he's away for a few weeks.  I hear Smaug's an events person, so he'd probably be a good choice.

Comment 18

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

Comment on attachment 269471 [details] [diff] [review]
unit test

>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
>+relativesrcdir  = docshell/test/chrome 
nit, align '=' properly

>+   - The Original Code is bug 364461 unit test
Unit or mochitest?

I like the way tests are iterated.

This tests a lot more than just this bug, but that is good. We need all kinds of event tests,
and load event is especially important.

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Do any of those tests cover the scenario in bug 384977?  Note that there it's navigation that is blocked, not just event firing...

Comment 20

[Sylvain Pasche]

They don't cover that case now, as I'm controlling navigation using loadURI/goBack/goForward. I could add/modify some tests to dispatch a click event to cover that case, and test if the location changed.

Comment 21

[Boris Zbarsky [:bzbarsky]]

If you could add a test to do that, that would be great!

Comment 22

[Sylvain Pasche]

Attachment: unit test v2

(In reply to comment #18)
> (From update of attachment 269471 [details] [diff] [review])
> >+DEPTH		= ../../..
> >+topsrcdir	= @top_srcdir@
> >+srcdir		= @srcdir@
> >+VPATH		= @srcdir@
> >+relativesrcdir  = docshell/test/chrome 
> nit, align '=' properly

There was a mix of spaces and tabs. I've replaced by tabs.

> >+   - The Original Code is bug 364461 unit test
> Unit or mochitest?

mochitest if fine for me ;-)

Thanks for the review.

(In reply to comment #19)
> Do any of those tests cover the scenario in bug 384977?  Note that there it's
> navigation that is blocked, not just event firing...

I've added a test from the testcase of bug 384977 comment 10

Comment 23

[Boris Zbarsky [:bzbarsky]]

Thanks for adding that test!  And I really like your use of iterators here.  I'll have to remember that for future tests along these lines!

Comment 27

[Daniel Veditz [:dveditz]]

FF 2.0.0.5 code-freeze approaching, if we want to get this in we need reviews and trunk-landing ASAP.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 268917 [details] [diff] [review]
Proposed fix

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

r=dveditz unless you really need biesi to look at this in addition to jst

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 268917 [details] [diff] [review]
Proposed fix

I'd really like biesi to look, yes.  This is somewhat fragile code, and I want the extra pair of eyes if we're going to take it on branch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk and branches.  On branches, I left out the InternalLoad hunk, which should really go in with bug 371360.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Checked in the tests too.  Thanks again for those!