Closed Bug 1001120 Opened 10 years ago Closed 10 years ago

[Session Restore] Remove the FrameTree Observer from content-sessionStore.js SessionHistoryListener

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: smacleod, Assigned: smacleod)

References

Details

(Whiteboard: p=1 s=it-32c-31a-30b.1 [qa-])

Attachments

(2 files)

After Bug 981900 lands using a frame tree observer in the SessionHistoryListener[1] is redundant. The new OnHistoryReplaceEntry should cover the case where we navigate away from an about page.


[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js?rev=d43d50820603#223
Flags: firefox-backlog?
Patch to remove the observer (also cleaned up a small comment I missed in the patch for Bug 981900).
Attachment #8412126 - Flags: review?(ttaubert)
Comment on attachment 8412126 [details] [diff] [review]
Patch - Remove the frame tree observer from the content-sessionStore.js SessionHistoryListener

Review of attachment 8412126 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet!
Attachment #8412126 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
Flags: firefox-backlog? → firefox-backlog+
Whiteboard: p=1 s=it-31c-30a-29b.3 [qa-]
This patch should fix the intermittent test failures introduced by the previous patch. We once again handle subframe load events.

Usually the child entries for these subframes would be collected due to the message queue delay we use for batching but when a subframe took longer than this delay to load it didn't have an entry yet.

I've also introduced a test for this which fails before the included change.
Attachment #8414022 - Flags: review?(ttaubert)
Comment on attachment 8414022 [details] [diff] [review]
Patch 2 - Handle subframe load events to collect slow loading subframes

Review of attachment 8414022 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the test (only), thanks for writing this!

The rest of the patch will be covered by a backout of the previous push as we agreed to over IRC.
Attachment #8414022 - Flags: review?(ttaubert) → review+
Pushed backout of "Patch" (a2d961fb4789):
https://hg.mozilla.org/integration/fx-team/rev/fd7ac192e4d8

Pushed introduction of test for what caused the intermittent oranges:
https://hg.mozilla.org/integration/fx-team/rev/a7e3d4484692
Whiteboard: p=1 s=it-31c-30a-29b.3 [qa-] → p=1 s=it-32c-31a-30b.1 [qa-]
https://hg.mozilla.org/mozilla-central/rev/a7e3d4484692
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Some more info on the decision here:

The intermittent orange was caused by us collecting shistory data when the first root shentry was created. Due to one or more iframes taking a long time to load those didn't show up yet in the shentry children.

Steven's patch would have fixed this by listening for subframe loads, which OTOH would have caused invalidations when dynamic frames are created or navigated - which we ignore since bug 936271.

Backing out and re-adding the FrameTree observer makes us collect when the root shentry with its children is ready but it also makes us invalidate twice for most of the actions handled by the nsISHistoryListener. Navigating back will fire onHistoryGoBack() and cause a FrameTree notification.

We discussed all this with future incremental updates in mind but realized that it's not as easy as setting shistory.index -= 1 when onHistoryGoBack() is called because due to how the FrameTree collection works we already discarded shistory children for the previous shentry.

For now it seems easier to just revert the change and use the FrameTree observer again. Steven is quite sure that there were cases where history.back()/forward() didn't cause a STATE_STOP notification when the target shentry is an artificial one as added by pushState() or replaceState().

When tackling incremental shistory updates we will need to cover all these fancy situations with lots of tests to make sure we don't get things wrong again.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.