| Summary: | WebTiming does not take page cache into account | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
| Component: | WebKit Misc. | Assignee: | Mihai Parparita <mihaip> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | abarth, dbates, eric, mihaip, tonyg, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Mac | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Bug Depends on: | 42775 | ||||||||||
| Bug Blocks: | 30685, 42434, 42435 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Simon Fraser (smfr)
2010-07-21 12:11:51 PDT
This prevents me from browser layout test results with a debug build. Yeah, this bug is super annoying to me too. Tonyg, are you looking at it? (In reply to comment #2) > Yeah, this bug is super annoying to me too. Tonyg, are you looking at it? Yes, I'll be landing a patch to temporarily comment out the ASSERT momentarily. I've disabled the ASSERT for now. The consequence of that ASSERT being reached is that the unloadEventEnd exposed to Web Timing is incorrect. I'm leaving this open to track the actual fix. Created attachment 63543 [details]
testcast
I've attached a reduced test case. For triggering the assert when going page 1-> page 2 -> back the requirements are for page 1 to not have an unload handler but for page 2 to have one. This makes page 1 end up in the page cache, while page 2 doesn't. Tony, do you mind if I work on fixing this? It seems like another good (i.e. simple) initial patch for me. (In reply to comment #6) > I've attached a reduced test case. For triggering the assert when going page 1-> page 2 -> back the requirements are for page 1 to not have an unload handler but for page 2 to have one. This makes page 1 end up in the page cache, while page 2 doesn't. > > Tony, do you mind if I work on fixing this? It seems like another good (i.e. simple) initial patch for me. By all means. Nice reduction :-) After looking at this a bit more, it looks like the WebTiming code in general doesn't take the page cache into account at all. When we're restoring a page from the page cache, we bypass the normal loading flow (e.g. navigationStart is called in continueLoadAfterWillSubmitForm, but we exit before that in continueLoadAfterNavigationPolicy, and instead call loadProvisionalItemFromCachedPage). The fix might be as simple as resetting the timing data in loadProvisionalItemFromCachedPage (so that times recorded for the previous time the page was displayed aren't used) and also recording navigationStart there. Created attachment 63783 [details]
Patch
Adam, can you review this (since you helped Tony and I understand what's going on in the first place)? Created attachment 63786 [details]
Patch
Comment on attachment 63786 [details]
Patch
Looks great! Thanks.
Comment on attachment 63786 [details] Patch Clearing flags on attachment: 63786 Committed r64939: <http://trac.webkit.org/changeset/64939> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/64939 might have broken GTK Linux 64-bit Debug This is the diff from the GTK builder of onunload-back-to-page-cache.html -main frame "navigated" - has 1 onunload handler(s) +main frame - has 1 onunload handler(s) Seems like the GTK DRT doesn't report frame names quite the same way as the other DRTs. I'll modify the test to clear the frame name before exiting and file a bug about the GTK DRT (if there isn't one already). > Seems like the GTK DRT doesn't report frame names quite the same way as the other DRTs. I'll modify the test to clear the frame name before exiting and file a bug about the GTK DRT (if there isn't one already).
Thanks. The other option is to make the Gtk DRT report frame names correctly.
(In reply to comment #17) > > Seems like the GTK DRT doesn't report frame names quite the same way as the other DRTs. I'll modify the test to clear the frame name before exiting and file a bug about the GTK DRT (if there isn't one already). > > Thanks. The other option is to make the Gtk DRT report frame names correctly. I filed bug 43690 about fixing the GTK DRT, but since I don't have a Linux build environment (yet), modifying the test seems like reasonable workaround for now (bug 43692 has that). |