967028 - history.pushState() and .replaceState() don't invalidate shistory

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

STR:

1) Open https://github.com/mozilla/gecko-dev
2) Click the "10,000+ commits" link at the body's top left.
3) Middle-click (or Cmd-Click) the reload button to duplicate the tab.

Actual:

We're back to the page from (1).

Expected:

We should see the list of commits, i.e. https://github.com/mozilla/gecko-dev/commits/master


The problem is that per spec pushState() and replaceState() don't emit any events. We should be able to cover that with nsISHistoryListener.OnHistoryNewEntry(). I wonder if we would still need to listen for "load" and "hashchange" then?

Comment 2

[Steven MacLeod [:smacleod]]

Attachment: Patch - Use a SHistoryListener to collect entries from pushState() and replaceState()

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8385011 [details] [diff] [review]
Patch - Use a SHistoryListener to collect entries from pushState() and replaceState()

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

For a follow-up: we should probably merge ContentRestore.jsm's history listener with the one introduced here.

Testing replaceState() and pushState() shouldn't be too hard. Can you please add a test?

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +219,2 @@
>    init: function () {
>      gFrameTree.addObserver(this);

I wonder, do we still need the frame tree observer part? It looks like all we care about is covered by the SHistoryListener? SHistory collection doesn't use the frame tree anyway.

@@ +255,5 @@
>      this.collect();
> +
> +    // We add ourselves as a SHistoryListener after the frame tree has been
> +    // collected to avoid firing OnHistoryNewEntry etc. for any history which
> +    // was restored.

It would certainly make the code simpler if we could just register a listener once and then forget about it. Is it really a problem when OnHistoryNewEntry() is called when restoring a tab? We have all kinds of invalidation listeners that are fired when restoring a tab - which is okay as the tab is now live again and we will want to collect data anyway.

@@ +284,5 @@
> +
> +  OnHistoryGotoIndex: function (index, gotoURI) {
> +    this.collect();
> +    return true;
> +  },

BTW: Good to have all those notifications set up already, all we need to do in the future here is to just send a new "index" to the parent, not sure how often that is fired though in everyday browsing. Certainly useful for back/fwd.

@@ +287,5 @@
> +    return true;
> +  },
> +
> +  QueryInterface: XPCOMUtils.generateQI([
> +     Ci.nsISHistoryListener,

To implement the whole interface, we will need to add OnHistoryReload() and OnHistoryPurge() as well. I don't think they're optional.

@@ +289,5 @@
> +
> +  QueryInterface: XPCOMUtils.generateQI([
> +     Ci.nsISHistoryListener,
> +     Ci.nsISupportsWeakReference
> +   ])

Nit: indentation is a little off here.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Testing replaceState() and pushState() shouldn't be too hard. Can you please
> add a test?

Extending browser_sessionHistory.js would totally do.

Comment 5

[Steven MacLeod [:smacleod]]

Attachment: Patch - Use a SHistoryListener to collect entries from pushState() v2

We're not going to wait on Bug 981900 to land so we can fix replaceState()
in this patch as well, because we'd like to uplift this and get pushState()
working at least. I've left the test for replaceState commented out and we
can enable it in the trunk patch which will follow up Bug 981900.

Even though Tim and I have discussed some of his feedback in IRC, I'm going to
answer here so the code can be reviewed while he's on PTO.

> I wonder, do we still need the frame tree observer part? It looks like all
> we care about is covered by the SHistoryListener? SHistory collection
> doesn't use the frame tree anyway.

Yes, we need the frame tree observer until Bug 981900 lands. We don't invalidate
on a navigation away from an about page without using the frame tree here. once
we have OnHistoryReplaceEntry in the SHistoryListener we can remove it.


> It would certainly make the code simpler if we could just register a
> listener once and then forget about it. Is it really a problem when
> OnHistoryNewEntry() is called when restoring a tab? We have all kinds of
> invalidation listeners that are fired when restoring a tab - which is okay
> as the tab is now live again and we will want to collect data anyway.

We do fire OnHistoryNewEntry for every history entry being restored, but the
overhead should be minimal. This micro optimization probably isn't worthwhile.
I've simplified the patch here.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8392633 [details] [diff] [review]
Patch - Use a SHistoryListener to collect entries from pushState() v2

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

Looks good to me, thanks.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +228,5 @@
>  
>    uninit: function () {
> +    let sessionHistory = docShell.QueryInterface(Ci.nsIWebNavigation).sessionHistory;
> +    if (sessionHistory) {
> +      sessionHistory.removeSHistoryListener(this);

In which case may this be null?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Steven MacLeod [:smacleod] from comment #5)
> > I wonder, do we still need the frame tree observer part? It looks like all
> > we care about is covered by the SHistoryListener? SHistory collection
> > doesn't use the frame tree anyway.
> 
> Yes, we need the frame tree observer until Bug 981900 lands. We don't
> invalidate
> on a navigation away from an about page without using the frame tree here.
> once
> we have OnHistoryReplaceEntry in the SHistoryListener we can remove it.


This doesn't seem documented in your patch. Can you document it?

> > It would certainly make the code simpler if we could just register a
> > listener once and then forget about it. Is it really a problem when
> > OnHistoryNewEntry() is called when restoring a tab? We have all kinds of
> > invalidation listeners that are fired when restoring a tab - which is okay
> > as the tab is now live again and we will want to collect data anyway.
> 
> We do fire OnHistoryNewEntry for every history entry being restored, but the
> overhead should be minimal. This micro optimization probably isn't
> worthwhile.

That too would be interesting to document.

Comment 8

[Steven MacLeod [:smacleod]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #6) 
> ::: browser/components/sessionstore/content/content-sessionStore.js
> @@ +228,5 @@
> >  
> >    uninit: function () {
> > +    let sessionHistory = docShell.QueryInterface(Ci.nsIWebNavigation).sessionHistory;
> > +    if (sessionHistory) {
> > +      sessionHistory.removeSHistoryListener(this);
> 
> In which case may this be null?

It seems to happen when exiting the browser with tabs open.

Comment 9

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/integration/fx-team/rev/d43d50820603

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d43d50820603

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Steven, can we uplift this to Aurora and Beta? I don't think there should be any bigger conflicts, right?

Comment 12

[Jenn Chaulk (:jchaulk)]

Hi Alexandra, I'm just looking for an update on when verification will be complete for this item, as our iteration ends on Monday, March 31st.

Comment 13

[Steven MacLeod [:smacleod]]

Comment on attachment 8392633 [details] [diff] [review]
Patch - Use a SHistoryListener to collect entries from pushState() v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 860903
User impact if declined: Session History will not be recollected when pushState() is called, possibly causing saved and restored history to be stale.
Testing completed (on m-c, etc.): On m-c and patch introduces tests for behavior. 
Risk to taking this patch (and alternatives if risky): Low risk
String or IDL/UUID changes made by this patch: None

Comment 14

[Cornel Ionce [:noni]]

Mozilla/5.0 (Windows NT 5.2; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Windows NT 6.3; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0

Tested using the STR from the description on latest Nightly (build ID: 20140325030201).
The tab is correctly duplicated by middle-clicking or by using "Ctrl"/"Cmd" + Click on the reload button; view changes to the opened tab.

Verified on Windows XP, Windows 7 64bit, Windows 8.1 32bit, Ubuntu 13.04 and Mac OS X 10.9

Comment 15

[Jenn Chaulk (:jchaulk)]

Hi Alexandra. Could you let me know when verification for this item will be complete?  Our iteration ends on Monday, March 31st.

Comment 16

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2eb3d41f2673

Comment 17

[Steven MacLeod [:smacleod]]

https://hg.mozilla.org/releases/mozilla-beta/rev/05c607e90fad

Comment 18

[Ioana (away)]

Updating per comment 14...

Comment 19

[Ada [:adalucinet]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:30.0) Gecko/20100101 Firefox/30.0 
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (Windows NT 6.3; rv:30.0) Gecko/20100101 Firefox/30.0

Reproduced this issue with Aurora from 2014-03-26.

Verified as fixed on latest Aurora (Build ID: 20140327004002) with STR from comment 0, on the following Operating Systems: Windows 7 64bit, Windows 8.1 32bit, Ubuntu 13.10 32bit and Mac OS X 10.8.

Verification on beta branch will be done tomorrow, after Firefox 29 beta 3 builds are available.

Comment 20

[Ada [:adalucinet]]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 5.2; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed with Firefox 29 beta 3 (Build ID: 20140327113732) on Windows 7 64bit, Windows XP 64bit, Ubuntu 12.04 32bit and Mac OS X 10.6.