491168 - Allow SessionStore to save/restore referrer field

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Michael Kraft [:morac]]

The referrer field for documents in tabs is currently not saved nor restored.  This includes the case where a tab is closed and then recovered via the "Recently Closed Tabs" menu .  It would be nice for this field to be saved in the case where a page is not cached and the page checks the referrer field before allowing the page to load.

As the document.referrer field is read only, this is not something that can be fixed in nsSessionStore.js alone.

Test case:

Go to URL and click on link on the page, close tab, reopen tab and refresh page.  Referrer is lost.

Comment 1

[Nochum Sossonko [:Natch]]

(In reply to comment #0)
> As the document.referrer field is read only, this is not something that can be
> fixed in nsSessionStore.js alone.

nsIWebNavigation (at least) has an argument to set the referrer field, not sure if this can be used in sessionstore though...

Comment 2

[Simon Bünzli]

(In reply to comment #0)
> As the document.referrer field is read only, this is not something that can be
> fixed in nsSessionStore.js alone.

The referrer can be set through the nsISHEntry interface which SessionStore already uses for (de)serializing history entries. This should be a three-line patch (and a few dozen lines for a test case).

Want to give it a try, Michael?

Comment 3

[Michael Kraft [:morac]]

(In reply to comment #2)
> The referrer can be set through the nsISHEntry interface which SessionStore
> already uses for (de)serializing history entries. This should be a three-line
> patch (and a few dozen lines for a test case).
> 
> Want to give it a try, Michael?

Okay.  I added the following to _serializeHistoryEntry after the storing of the ID:

    if (aEntry.referrerURI)
      entry.referrerURI = aEntry.referrerURI;

and the following to _deserializeHistoryEntry after the restoring of the contentType:

    if (aEntry.referrerURI)
      shEntry.referrerURI = aEntry.referrerURI;


This appears to work in the case of closing and opening tabs, but if I try to restart Minefield or load a saved session on the nightly trunk load, Minefield crashes on "browser.webNavigation.gotoIndex(activeIndex);" in restoreHistory:

http://crash-stats.mozilla.com/report/index/979afebe-87ee-462c-b49e-3c6c82090503


In that case aEntry.referrerURI is defined as an object instead of an instance of a nsiURL so that's probably why it's crashing.  The crash should probably be filed as a bug in any case since it results when messing with the referrerURI in session history, but I can't come up with a method of recreating it without modifying nsSessionStore.js.


In any case I need to find some way of taking the aEntry.referrerURI and converting it into an instance of nsiURI (and making sure it's valid so it doesn't cause a crash).  Any ideas?

Comment 4

[Simon Bünzli]

(In reply to comment #3)
>     if (aEntry.referrerURI)
>       entry.referrerURI = aEntry.referrerURI;

For serializing, you rather want the aEntry.referrerURI.spec

> The crash should probably be filed as a bug in any case

Indeed, please do so.

> In any case I need to find some way of taking the aEntry.referrerURI and
> converting it into an instance of nsiURI

That's what ioService.newURI should do (which we already use for restoring the visited URLs themselves).

Comment 5

[Michael Kraft [:morac]]

(In reply to comment #4)
> (In reply to comment #3)
> >     if (aEntry.referrerURI)
> >       entry.referrerURI = aEntry.referrerURI;
> 
> For serializing, you rather want the aEntry.referrerURI.spec
> 

I went to post that I had changed it to the following which works, but collided with your post.
shEntry.referrerURI = ioService.newURI(aEntry.referrerURI.spec, null, null);

So apparently I figured this out correctly.

> > The crash should probably be filed as a bug in any case
> 
> Indeed, please do so.
> 

The problem I'm having is that I don't know how to reproduce this problem without making changes to nsSessionStore.js.  I could file it without a test case, but it would be better if it had one.

Comment 6

[Michael Kraft [:morac]]

I filed the crash as bug 491204, feel free to comment on it if you have any additional insight.

Now I just need to get a test case working.  I'm assuming we'd want to test actually saving and restoring a session and not just closing and restoring a tab or window, correct?

Comment 7

[Simon Bünzli]

(In reply to comment #5)
> shEntry.referrerURI = ioService.newURI(aEntry.referrerURI.spec, null, null);

You should get the spec already at serializing, so that we don't carry any nsIURIs around internally.

(In reply to comment #6)
> I'm assuming we'd want to test actually saving and restoring a session and
> not just closing and restoring a tab or window, correct?

What about:
1. Load a page with a referrer.
2. Make sure that referrer is saved for getTabState.
3. Set a different referrer through setTabState.
4. Ensure the page's document.referrer has been correctly changed.
5. Bonus points for closing/reopening or duplicating the tab.

That should sufficiently test the added code path. No need for saving/restoring a whole session...

Comment 8

[Michael Kraft [:morac]]

(In reply to comment #7)
> You should get the spec already at serializing, so that we don't carry any
> nsIURIs around internally.

Yeah, I realized that was kind of stupid and had already changed it after posting.

> What about:
> 1. Load a page with a referrer.
> 2. Make sure that referrer is saved for getTabState.
> 3. Set a different referrer through setTabState.
> 4. Ensure the page's document.referrer has been correctly changed.
> 5. Bonus points for closing/reopening or duplicating the tab.

Okay I got a test that does all this finished and working.  It took me a lot longer than it should have, but it took me a while to figure out how to delay between the steps to give time for the referrer to propagate.

I'll upload shortly.

Comment 9

[Michael Kraft [:morac]]

Attachment: Patch and test files

Comment 10

[Simon Bünzli]

Comment on attachment 375557 [details] [diff] [review]
Patch and test files

Looking good.

>+    if (aEntry.referrerURI)
>+      entry.referrerURISpec = aEntry.referrerURI.spec;

Please use referrerUrl or even just referrer instead of the overlong referrerURISpec.

>+ * The Initial Developer of the Original Code is
>+ * Simon Bünzli <[e-mail]>.

Wouldn't that be you?

>+  const REFER1 = "http://www.example.net/?" + Date.now(), REFER2 = "http://www.example.net/?" + Math.random();

Nit: Please complete the names and make it one variable declaration per line.

>+  browser.addEventListener("load", function loadListener(e) {

If you want to explicitly list the function's argument, please name it aEvent.

>+    ss.setTabState(tab, JSON.stringify(tabState));
>+
>+    executeSoon(function() {

That looks too fragile. Please wait for the SSTabRestored notification.

>+  browser.loadURI("http://www.example.net", referrerURI, null);

Since you don't need referrerURI before, please don't declare it until right above this line.

r+=me with those changes.

Comment 11

[Michael Kraft [:morac]]

(In reply to comment #10)
> Please use referrerUrl or even just referrer instead of the overlong
> referrerURISpec.

Okay I'll change that.

> Wouldn't that be you?

I originally copied the test code from another test that you wrote, but ended up completely rewriting it so, yeah I guess it should be me.

> Nit: Please complete the names and make it one variable declaration per line.

What exactly do you mean by "complete the names"?
Do you mean use "REFERRER" instead of "REFER"?

> If you want to explicitly list the function's argument, please name it aEvent.

Done

> That looks too fragile. Please wait for the SSTabRestored notification.

Done

> Since you don't need referrerURI before, please don't declare it until right
> above this line.

Okay

Comment 12

[Simon Bünzli]

(In reply to comment #11)
> Do you mean use "REFERRER" instead of "REFER"?

Yes.

Comment 13

[Michael Kraft [:morac]]

Attachment: Patch and test files 2.0

Made requested changes.  Submitting for superreview.  Is that even necessary for this patch?

Comment 14

[Simon Bünzli]

Comment on attachment 375684 [details] [diff] [review]
Patch and test files 2.0

In browser code, superreview is only needed for API changes.

Comment 15

[Michael Kraft [:morac]]

Ah okay then.

Since the patch is so trivial and since I filed the bug on behalf of a few Session Manager users, I'd like if it could make it into Firefox 3.5.  I'm not sure what the procedure for doing so is exactly.  Do I just "?" the wanted-firefox3.5 flag or do I "?" the approval1.9.1 flag?  Does it have to land on the trunk before I do so?

Comment 16

[Simon Bünzli]

(In reply to comment #15)
> I'd like if it could make it into Firefox 3.5.

The patch will first have to land and "bake" on Trunk. If no issues emerge (i.e. if the tree remains green and there are no perf regressions), you can set the approval1.9.1? flag and summarize for the branch drivers, why this patch should still be taken at this late point in the game.

Comment 17

[Michael Kraft [:morac]]

It's been a while since I tagged this "checkin-needed". Any chance this could pushed to the Trunk?

Comment 18

[Michael Kohler [:mkohler]]

(In reply to comment #17)
> It's been a while since I tagged this "checkin-needed". Any chance this could
> pushed to the Trunk?

Since this is nothing for Firefox 3.5 you'll have to wait till the restriction for mozilla-central is over. Please see <http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/15a349fecbfaf701/f414ce5077ff01a4> for further information.

Comment 19

[Michael Kraft [:morac]]

Ah okay I didn't realize there was a freeze (though it didn't start until 2 weeks after I originally requested a check-in).

Comment 20

[Dão Gottwald [::dao]]

http://hg.mozilla.org/mozilla-central/rev/0c5edb1d5db1