Last Comment Bug 364461 - [FIX]Pagehide not always dispatched in subframes
: [FIX]Pagehide not always dispatched in subframes
Status: RESOLVED FIXED
: fixed1.8.0.13, fixed1.8.1.5, testcase
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla1.9alpha8
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
: 384824 384977 386276 387074 (view as bug list)
Depends on:
Blocks: CVE-2007-1095
  Show dependency treegraph
 
Reported: 2006-12-20 05:26 PST by Sylvain Pasche
Modified: 2008-07-31 02:49 PDT (History)
11 users (show)
mbeltzner: blocking1.9+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase subframe (240 bytes, text/html)
2006-12-20 05:28 PST, Sylvain Pasche
no flags Details
main testcase (810 bytes, text/html)
2006-12-20 05:29 PST, Sylvain Pasche
no flags Details
hack which solves the issue (for demonstration) (1.14 KB, patch)
2007-01-02 12:38 PST, Sylvain Pasche
no flags Details | Diff | Review
version 1 (1.90 KB, patch)
2007-02-25 13:20 PST, Sylvain Pasche
no flags Details | Diff | Review
Proposed fix (2.07 KB, patch)
2007-06-18 23:04 PDT, Boris Zbarsky [:bz]
dveditz: review+
cbiesinger: review+
jst: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Review
unit test (13.81 KB, patch)
2007-06-22 19:02 PDT, Sylvain Pasche
bugs: review+
Details | Diff | Review
unit test v2 (16.67 KB, patch)
2007-06-28 14:34 PDT, Sylvain Pasche
no flags Details | Diff | Review

Description Sylvain Pasche 2006-12-20 05:26:50 PST
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 2006-12-20 05:28:00 PST
Created attachment 249246 [details]
testcase subframe
Comment 2 Sylvain Pasche 2006-12-20 05:29:38 PST
Created attachment 249247 [details]
main testcase
Comment 3 Sylvain Pasche 2007-01-02 12:38:26 PST
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 ;-) )
Comment 4 Sylvain Pasche 2007-01-21 14:07:07 PST
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 2007-02-25 13:20:14 PST
Created attachment 256377 [details] [diff] [review]
version 1

Clearing the mFiredUnloadEvent flag in FinishRestore() solves this specific issue.
Comment 6 Boris Zbarsky [:bz] 2007-02-25 13:39:10 PST
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 2007-02-25 13:57:18 PST
(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 8 Boris Zbarsky [:bz] 2007-06-18 22:34:54 PDT
*** Bug 384977 has been marked as a duplicate of this bug. ***
Comment 9 Boris Zbarsky [:bz] 2007-06-18 22:38:36 PDT
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 [:bz] 2007-06-18 23:03:12 PDT
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 [:bz] 2007-06-18 23:04:50 PDT
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.
Comment 12 Boris Zbarsky [:bz] 2007-06-18 23:05:54 PDT
Comment on attachment 268917 [details] [diff] [review]
Proposed fix

We need this on branch if we take bug 371360.
Comment 13 Sylvain Pasche 2007-06-19 16:33:29 PDT
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 [:bz] 2007-06-19 17:19:49 PDT
If it reproduces the bug, that sounds great!
Comment 15 Daniel Veditz [:dveditz] 2007-06-21 14:24:05 PDT
wanted on the branch, but not stop-ship (unless we missed something about why it was nominated)
Comment 16 Sylvain Pasche 2007-06-22 19:02:19 PDT
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?
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2007-06-23 16:36:28 PDT
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] 2007-06-28 01:58:25 PDT
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 [:bz] 2007-06-28 09:23:05 PDT
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 2007-06-28 09:44:07 PDT
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 [:bz] 2007-06-28 09:55:05 PDT
If you could add a test to do that, that would be great!
Comment 22 Sylvain Pasche 2007-06-28 14:34:26 PDT
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
Comment 23 Boris Zbarsky [:bz] 2007-06-28 14:46:25 PDT
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 24 Boris Zbarsky [:bz] 2007-06-28 19:12:30 PDT
*** Bug 386276 has been marked as a duplicate of this bug. ***
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-07-05 14:10:12 PDT
*** Bug 384824 has been marked as a duplicate of this bug. ***
Comment 26 Boris Zbarsky [:bz] 2007-07-06 04:59:19 PDT
*** Bug 387074 has been marked as a duplicate of this bug. ***
Comment 27 Daniel Veditz [:dveditz] 2007-07-09 10:29:02 PDT
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] 2007-07-09 10:54:12 PDT
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 [:bz] 2007-07-09 13:11:36 PDT
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 [:bz] 2007-07-11 10:36:55 PDT
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 [:bz] 2007-07-11 10:38:48 PDT
Checked in the tests too.  Thanks again for those!

Note You need to log in before you can comment on or make changes to this bug.