[FIX]Pagehide not always dispatched in subframes

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
Document Navigation
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Sylvain Pasche, Assigned: bz)

Tracking

({fixed1.8.0.13, fixed1.8.1.5, testcase})

Trunk
mozilla1.9alpha8
x86
Linux
fixed1.8.0.13, fixed1.8.1.5, testcase
Points:
---
Bug Flags:
blocking1.9 +
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 249246 [details]
testcase subframe
(Reporter)

Comment 2

11 years ago
Created attachment 249247 [details]
main testcase
(Reporter)

Comment 3

11 years ago
Created attachment 250200 [details] [diff] [review]
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 ;-) )
(Reporter)

Comment 4

11 years ago
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.
Status: NEW → ASSIGNED
(Reporter)

Comment 5

10 years ago
Created attachment 256377 [details] [diff] [review]
version 1

Clearing the mFiredUnloadEvent flag in FinishRestore() solves this specific issue.
Attachment #250200 - Attachment is obsolete: true
Attachment #256377 - Flags: review?(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?
(Reporter)

Comment 7

10 years ago
(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.
(Reporter)

Updated

10 years ago
Attachment #256377 - Flags: review?(bzbarsky)
Duplicate of this bug: 384977
Blocks: 371360
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.
Flags: blocking1.9?
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
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.
Flags: in-testsuite?
Keywords: qawanted
Created attachment 268917 [details] [diff] [review]
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.
Assignee: nobody → bzbarsky
Attachment #256377 - Attachment is obsolete: true
Attachment #268917 - Flags: superreview?(jst)
Attachment #268917 - Flags: review?(cbiesinger)
Summary: Pagehide not always dispatched in subframes → [FIX]Pagehide not always dispatched in subframes
Target Milestone: --- → mozilla1.9alpha6
Comment on attachment 268917 [details] [diff] [review]
Proposed fix

We need this on branch if we take bug 371360.
Attachment #268917 - Flags: approval1.8.1.5?
Attachment #268917 - Flags: approval1.8.0.13?

Updated

10 years ago
Attachment #268917 - Flags: superreview?(jst) → superreview+
(Reporter)

Comment 13

10 years ago
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?
If it reproduces the bug, that sounds great!
wanted on the branch, but not stop-ship (unless we missed something about why it was nominated)
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
(Reporter)

Comment 16

10 years ago
Created attachment 269471 [details] [diff] [review]
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?
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.

Updated

10 years ago
Attachment #269471 - Flags: review?(Olli.Pettay)

Comment 18

10 years ago
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
Attachment #269471 - Flags: review?(Olli.Pettay) → review+
Do any of those tests cover the scenario in bug 384977?  Note that there it's navigation that is blocked, not just event firing...
(Reporter)

Comment 20

10 years ago
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.
If you could add a test to do that, that would be great!
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Whiteboard: need r=biesi
(Reporter)

Comment 22

10 years ago
Created attachment 270227 [details] [diff] [review]
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
Attachment #269471 - Attachment is obsolete: true
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!
Duplicate of this bug: 386276
Whiteboard: need r=biesi → need r=biesi .
Whiteboard: need r=biesi . → need r=biesi and trunk landing
Duplicate of this bug: 384824
Duplicate of this bug: 387074
FF 2.0.0.5 code-freeze approaching, if we want to get this in we need reviews and trunk-landing ASAP.
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
Attachment #268917 - Flags: review?(cbiesinger)
Attachment #268917 - Flags: review+
Attachment #268917 - Flags: approval1.8.1.5?
Attachment #268917 - Flags: approval1.8.1.5+
Attachment #268917 - Flags: approval1.8.0.13?
Attachment #268917 - Flags: approval1.8.0.13+
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.
Attachment #268917 - Flags: review?(cbiesinger)
Attachment #268917 - Flags: review?(cbiesinger) → review+
Checked in on trunk and branches.  On branches, I left out the InternalLoad hunk, which should really go in with bug 371360.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: blocking1.9?
Keywords: qawanted → fixed1.8.0.13, fixed1.8.1.5
Resolution: --- → FIXED
Whiteboard: need r=biesi and trunk landing
Checked in the tests too.  Thanks again for those!
Flags: blocking1.9+

Updated

9 years ago
Component: History: Session → Document Navigation
QA Contact: history.session → docshell
You need to log in before you can comment on or make changes to this bug.