240 bytes, text/html
810 bytes, text/html
2.07 KB, patch
|Details | Diff | Splinter Review|
16.67 KB, patch
|Details | Diff | Splinter Review|
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.
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 ;-) )
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.
Created attachment 256377 [details] [diff] [review] version 1 Clearing the mFiredUnloadEvent flag in FinishRestore() solves this specific issue.
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?
(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.
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.
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.
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 on attachment 268917 [details] [diff] [review] Proposed fix We need this on branch if we take bug 371360.
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)
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.
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
Do any of those tests cover the scenario in bug 384977? Note that there it's navigation that is blocked, not just event firing...
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!
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
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!
FF 126.96.36.199 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 188.8.131.52 and 184.108.40.206, a=dveditz for release-drivers r=dveditz unless you really need biesi to look at this in addition to jst
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.
10 years ago
Checked in on trunk and branches. On branches, I left out the InternalLoad hunk, which should really go in with bug 371360.
Checked in the tests too. Thanks again for those!