|Summary:||Assertion failure when going back inside frame during onload|
|Product:||WebKit||Reporter:||Mihai Parparita <mihaip>|
|Component:||History||Assignee:||Mihai Parparita <mihaip>|
|Severity:||Normal||CC:||beidson, commit-queue, fishd|
|Version:||528+ (Nightly build)|
Description Mihai Parparita 2010-08-18 16:53:43 PDT
Comment 1 Mihai Parparita 2010-08-18 17:10:53 PDT
+Brady since this only happens in Safari/DRT, not in Chromium/TestShell, presumably because HISTORY_ALWAYS_ASYNC is set to 1 in the latter.
Comment 2 Mihai Parparita 2010-08-18 18:41:50 PDT
As best as I can tell, this is the ordering of what's happening when history.back() is synchronous: FrameLoader::checkCompleted() checkCallImplicitClose() Document::implicitClose dispatchWindowLoadEvent (skipping over JS bindings/JIT): History::back RedirectScheduler::scheduleHistoryNavigation HistoryController::goToItem HistoryController::recursiveGoToItem check that m_previousItem is cleared <-- checkLoadComplete() recursiveCheckLoadComplete() calls checkLoadCompleteForThisFrame() frameLoadCompleted() history()->updateForFrameLoadCompleted() clears m_previousItem The check indicated by the <-- happens before the actual clearing. One possible fix is for the "goToItem should be called synchronously" check in scheduleHistoryNavigation to not just look at the documentSequenceNumber of the root of the HistoryItem, but also to recursively compare all of the children frames' itemSequenceNumbers, so that we only do the call synchronously if we really are doing just a "state object traversal or a fragment traversal" (as the comment says). However, I'm not sure what the intention is with the synchronous history.back(). I spent some time reading bug 33538, but it was also interesting to see that all the fast/history/stateobjects layout tests still pass even with asynchronous history.back(). Was there a reason why the original test case in the bug was never landed?
Comment 3 Mihai Parparita 2010-08-19 16:47:37 PDT
Created attachment 64910 [details] Patch
Comment 4 Mihai Parparita 2010-08-19 16:51:45 PDT
The attached patch fixes this assert by (I believe) staying close to the sprit of the "navigation should be synchronous" check in RedirectScheduler::scheduleHistoryNavigation by also having it check the document sequence numbers. The test cases no longer crashes. Brady, do you have time to review this?
Comment 5 WebKit Commit Bot 2010-08-19 20:57:37 PDT
Comment on attachment 64910 [details] Patch Clearing flags on attachment: 64910 Committed r65724: <http://trac.webkit.org/changeset/65724>
Comment 6 WebKit Commit Bot 2010-08-19 20:57:42 PDT
All reviewed patches have been landed. Closing bug.