Closed Bug 670318 Opened 13 years ago Closed 13 years ago

Google+ home page creates multiple SHEntries, and if you go back to the first one, you can no longer go forward

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla8
Tracking Status
firefox7 + fixed
firefox8 + fixed

People

(Reporter: jaws, Assigned: ttaubert)

References

Details

(Keywords: regression, Whiteboard: [qa!])

Attachments

(3 files, 2 obsolete files)

When using Google+, the back button occassionally stops working. The Error Console shows these exceptions:

Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHistory.getEntryAtIndex]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/browser.js :: FillHistoryMenu :: line 8685"  data: no]

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHistory.getEntryAtIndex]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///components/nsSessionStore.js :: sss_collectTabData :: line 1670"  data: no]
Source File: resource:///components/nsSessionStore.js
Line: 1670

I have attached a short video of the reproduction (mp4, apologies for the patent-encumbered format).
> The Error Console shows these exceptions

Hm.  The second of those is reported in bug 669196.

The first is clearly a problem, though...

Olli, do you think you can take a look at this?
Blocks: 669196
Jared, per comments in bug 669196, Olli doesn't have a google+ account.  Not sure whether you're willing to let him use yours to reproduce the problem....
(In reply to comment #2)
> Jared, per comments in bug 669196, Olli doesn't have a google+ account.  Not
> sure whether you're willing to let him use yours to reproduce the problem....

I have sent you and Olli account details for Google+. Please let me know if there is anything more that I can do to help.
Steps to reproduce would be great.
(I don't have any software on this laptop which could play mp4)
I ran into this issue again today after I had joined a "hangout" for about 10 minutes and then exited the "hangout". I don't recall doing any other browsing on the site. Maybe this could be the cause?
I should also add that I do not have any extensions running. These are the list of plugins installed and enabled:

Shockwave Flash

    File: NPSWF32.dll
    Version: 10.3.181.34
    Shockwave Flash 10.3 r181

MIME Type 	Description 	Suffixes
application/x-shockwave-flash 	Adobe Flash movie 	swf
application/futuresplash 	FutureSplash movie 	spl
Google Talk Plugin

    File: npgoogletalk.dll
    Version: 2.1.8.0
    Version 2.1.8.0

MIME Type 	Description 	Suffixes
application/googletalk 	Google voice and video chat 	googletalk
Google Talk Plugin Video Accelerator

    File: npgtpo3dautoplugin.dll
    Version: 0.1.44.5
    Google Talk Plugin Video Accelerator version:0.1.44.5

MIME Type 	Description 	Suffixes
application/vnd.gtpo3d.auto 		
Google Update

    File: npGoogleUpdate3.dll
    Version: 1.3.21.57
    Google Update

MIME Type 	Description 	Suffixes
application/x-vnd.google.update3webcontrol.3 		
application/x-vnd.google.oneclickctrl.9 		
Silverlight Plug-In

    File: npctrl.dll
    Version: 4.0.60531.0
    4.0.60531.0

MIME Type 	Description 	Suffixes
application/x-silverlight 	npctrl 	scr
application/x-silverlight-2
I can confirm the problem. I'm using the latest nightly on GNU/Linux ( Mozilla/5.0 (X11; Linux i686; rv:8.0a1) Gecko/20110719 Firefox/8.0a1 ). It occurs also with a new profile. When I log in into Google+ and click on some inner link (for example, consult notifications), history is broken: I can click on the back or reload buttons, use the shortcuts, nothing happens. The error console is full of:

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsISHistory.getEntryAtIndex]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource:///components/nsSessionStore.js :: sss_collectTabData :: line 1668"  data: no]
Source File: resource:///components/nsSessionStore.js
Line: 1668

Everything works fine in other tabs I open.
I tried to run this simple snippet of code in Scratchpad:

var history = gBrowser.sessionHistory;
for (var j = 0; j < history.count; j++) {
  // history.count = 13
  var entry = null;
  try {
    entry = history.getEntryAtIndex(j, false);
  } catch (e) {
    // => exceptions for j > 2 in this particular case
  }
}

I think Google+ uses the History API and something goes wrong. But it's hard to reproduce. I restarted my browser 2 or 3 times and now everything is OK.
> When I log in into Google+ and click on some inner link (for example, consult 
> notifications), history is broken: I can click on the back or reload buttons, 
> use the shortcuts, nothing happens.

I was able to reproduce this once, but not consistently.  This bug worries me...
The sss_collectTabData assertions don't appear until some time after this bug manifests (presumably not until sessionrestore tries to save the tab data).

This seems to only happen after browsing around for a while on G+.  I wonder if I need to hit some race condition, where I call pushState at just the right time during a load.

Immediately after the issue manifest, I was able to right-click on the back button and get a drop-down list of history entries.  But now right-clicking does nothing.  Refresh also doesn't work.
The back button seems to stop working with some consistency if I click "notifications" as soon as it appears onscreen (while the page is still loading).

This is going to be fun...
Summary: Back button enabled but not working, drop down menu is empty → If you click a link on Google+ before the page has loaded, the back button may stop working
In a debug build, I get

> WARNING: NS_ENSURE_TRUE(txToRemove) failed: file ../../../../src/docshell/shistory/src/nsSHistory.cpp, line 1259

when the back button is active (i.e., not grayed-out) but when clicking it doesn't do anything.
Erm, I mean, I get that message when I click the back button, when it's not grayed out but when clicking it does nothing.
That is just a warning and not a sign of a bug itself.

But does the following fire (if put to line 1258):
NS_ASSERTION((!root1 && !root2) || (txToRemove && txToKeep), "Whaat!")
But anyway, I wonder if the session history is cleaning up itself too
aggressively.
No, that assertion didn't fire after I added it.

I did, however, get a bunch of new warnings this time:

WARNING: NS_ENSURE_SUCCESS(rv, 0) failed with result 0x8000FFFF: file ../../../../src/content/base/src/nsContentUtils.cpp, line 2528  (repeated many times)

###!!! ASSERTION: aToIndex is out of range: 'aToIndex < mLength', file ../../../../src/docshell/shistory/src/nsSHistory.cpp, line 867

WARNING: NS_ENSURE_TRUE(mOwner && !mCanceled) failed: file ../../../../src/modules/libpr0n/src/imgRequestProxy.cpp, line 574 (repeated many times)

WARNING: Subdocument container has no frame: file ../../../src/layout/base/nsDocumentViewer.cpp, line 2421

WARNING: NS_ENSURE_TRUE(currentHE) failed: file ../../../src/docshell/base/nsDocShell.cpp, line 3426

I'll attach a full log in a moment, in case I missed anything interesting.
nsSHistory.cpp, line 867 is interesting.
The push/replaceState code currently operates exclusively on mOSHE.  It seems likely that this is *a* problem, if not the problem.
I can reproduce it on demand now.

Go to http://forums.somethingawful.com/showthread.php?threadid=3397539&userid=0&perpage=30&pagenumber=1

Click "last"

Wait ~20 seconds (until SS fires? something in the page? dunno)

Try to reload/go back; wrinkle forehead in frustration.
I can't reproduce.  Are you logged into the forum?  Do you click "last" before the page finishes loading, or do you wait for it to load?
That page does, strangely enough, create 4 new shentries (so 5 total), even before I click "last".  And after I click "last", all but the first of those shentries corresponds to the new page, so I have to click back 5 times to get back to the first page.

This could be a bug in the site, but something similar happens for me sometimes on G+ (I have to click back multiple times to actually get back).
When I load https://plus.google.com in an unpatched browser, I get 4 shentries, all to the same URL, once the page finishes loading.  So this is similar to the behavior I see on Something Awful.

When I click back, the https box next to the favicon which says "google.com" disappears, even though the URL still says https://plus.google.com.

Something Awful and Google+ yield just one shentry when I load them in Chrome.
Confirmed the same behavior on G+ and Something Awful on Aurora and Beta.  At least this isn't a recent regression...
The Something Awful page creates multiple history entries with FF4, but not with FF3.6.9.  So something changed, but it could be that the page runs different script on 3.6.9 and 4.
Confirmed that Something Awful isn't calling push/replaceState, so something else is going on here.
I filed bug 673467 for Something Awful creating more SHEntries than it should, because it seems plausible that it's a different issue from the session history freezing.
Hmm, I can't reproduce the problem in Aurora, but can in nightlies.  I am indeed logged in.  Spend the $10 to get an account and expense it. :-)
> Spend the $10 to get an account and expense it. :-)

Just did, still can't reproduce after trying a few times.  Can you see if the patch in bug 673467 fixes the problem for you?  I can push to try if you don't have a tree handy.
Here's a better STR for G+:

* In a new tab, load https://plus.google.com  This creates 4 SHEntries for me now, but I've seen 5 before.

* Click <back> 3 times, so you're at the beginning of the G+ SHEntries.

* Click <forward>.  Nothing happens; you can verify that you're not taken forward in the history by right-clicking on the forward button and observing your current place in the history.

Notice also that after going back to the beginning of the SHistory, there are now only 2 SHEntries.  I think this is related to bug 673467 -- when I run with that patch, there are only ever 2 SHEntries for G+.  But the same bug manifests with the patch -- click back and then forward doesn't work.
Two of the three extra G+ shentries are due to bug 673467, but one isn't.  That one appears to come from G+ loading [1] in an iframe, and then, after its onload handler fires, loading [2].

It seems like we're doing the right thing by creating a SHEntry for this load, since it's an actual navigation, for all intents and purposes.  I'll post a testcase which demonstrates that we make a SHEntry in this case, while Chrome doesn't.

[1] https://talkgadget.google.com/talkgadget/blank
[2] https://talkgadget.google.com/talkgadget/notifierclient?blah-blah-blah
Summary: If you click a link on Google+ before the page has loaded, the back button may stop working → Google+ home page creates multiple SHEntries, and if you go back to the first one, you can no longer go forward
Attached file Testcase (obsolete) —
Ah, interesting!  This testcase creates 2 shentries in Firefox, and one in Chrome, as promised.

But it also is buggy in Firefox.  In a new tab, load http://mozilla.org.  Then load the testcase.  Click back until you go back to mozilla.org.  Now click forward.  You should be able to go forward twice, but in fact you can only go forward once!
I wouldn't use Chrome as an example how session history should work
since in general Webkit's session history is, or at least used to be 
very badly implemented.
IE has had quite good implementation.
Yes, I think in this case, WebKit is wrong (*).  G+ creates two SHEntries on IE9, one of which is related to talkgadget.  Apparently IE9 doesn't support data: URIs on iframes, so the testcase I attached doesn't work.

When I modified the testcase to use two http: URIs, you get two history entries (as with FF).  When you click back twice (so you're off the page) and then click forward, you're taken to the *second* page of the testcase -- presumably, IE doesn't have a bfcache, so when I go forward, it re-runs the inline script in the <head>.

In any case, IE's behavior seems sensible here, while FF's is clearly bugged.

(*) I need to check the spec to see what, if anything, it says about coalescing loads which happen before onload into a single history entry.
In response to (*) above, it doesn't appear that the spec says anything about coalescing loads.  At least, I'd expect to see that spec'ed as "traverse the session history with replacement enabled", and "replacement enabled" is only in a few places.
I was about to file another bug, but this seems to cover it. FWIW, I can reliably repro the issue on the SA forums without being logged in. And there appears to be 2 issues.

1. when reloading, history.index > history.count
2. after navigating, history.count lies (which is what was covered in bug 669196 and you seem to have covered).

Let me know if there's anything I can do to help.
Comment on attachment 547824 [details]
Testcase

I've split off the weirdness around this testcase into bug 674302.  That bug may end up being the same as this one; I'm not sure yet.
Attachment #547824 - Attachment is obsolete: true
Attached patch patch v1 (obsolete) — Splinter Review
Alas, I wasn't able to come up with a testcase, but I have the following STR (which is quite the same as Paul's):

1) Go to http://plus.google.com/
2) Wait for the page to load
3) Check that there are now 4 history entries
4) Press Ctrl+F5
5) Click any link (e.g. a user profile)
6) Wait for the error to appear

The attached patch fixes it. I think is a regression from bug 534178:

We have 4 history entries (so nsSHistory.mIndex is 3). Now LoadEntry() is called with mIndex=3 because we're reloading the page, this also sets mRequestedIndex=3. Now RemoveEntries() detects some duplicate history entries, removes them and decreases mIndex by one (for every duplicate) but doesn't touch mRequestIndex (which it should). Now after all duplicate entries were removed UpdateIndex() is called and sets mIndex=mRequestedIndex. The next function willing to work with the broken mIndex is going to fail.

I hope my explanation was correct because I'm not too familiar with that code :)
Attachment #549805 - Flags: review?(bzbarsky)
Attachment #549805 - Flags: review?(bzbarsky) → review+
I think bug 674302 should be fixed by this, which means we also have a testcase.

Tim, do you want to write a mochitest using what I have in bug 674302 and test that it fails without this patch?
(In reply to comment #40)
> Tim, do you want to write a mochitest using what I have in bug 674302 and
> test that it fails without this patch?

Sure, I'll do that.
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
Test added and ensured that it fails without the patch applied.
Attachment #549805 - Attachment is obsolete: true
Attachment #549861 - Flags: review?(Olli.Pettay)
Attachment #549861 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/968f8f50e428
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla8
STR is reproducible on Aurora, not in Beta.
Comment on attachment 549861 [details] [diff] [review]
patch v2 (with test)

Nominating for approval-mozilla-aurora.  This breaks navigation in a hard way on a popular site, and is apparently a regression from beta.  The fix is simple, although this being shistory, who knows if it's safe.
Attachment #549861 - Flags: approval-mozilla-aurora?
Justin / Tim / QA: Can we find the bug that regressed this in aurora (or exposed a latent issue)? Thanks!
(In reply to Christian Legnitto [:LegNeato] from comment #47)
> Justin / Tim / QA: Can we find the bug that regressed this in aurora (or
> exposed a latent issue)? Thanks!

mozregression printed the following range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d799c168c98d&tochange=3d475b322365

I bisected it further down to this changeset:

changeset:   70744:580ddd2f1267
user:        Kyle Huey <khuey@kylehuey.com>
date:        Tue Jun 07 13:04:13 2011 -0700
summary:     Bug 662200: Update the session history object for reloads. r=bz

https://hg.mozilla.org/mozilla-central/rev/580ddd2f1267
Blocks: 662200
Ok, so I think this is what happened:

1. Bug 622315 landed and caused bug 662200
2. Bug 622315 was backed out
3. Then bug 622315 relanded as well as a fix for bug 662200
4. The fix for bug 662200 caused this bug

Sound right? This code seems...brittle. Also concerned with the "who knows if it's safe". Do we just want to back out bug 622315 and 662200, thus making this bug not occur in Aurora? It seems like the assertion + leak fix in bug 622315 isn't worth the risk to me at first glance.
> This code seems...brittle

That's because it is.

Backing bug 622315 and bug 662200 out of aurora seems like the safe course of action to me as well...
(In reply to Boris Zbarsky (:bz) from comment #50)
> > This code seems...brittle
> 
> That's because it is.
Partly, or largely because session history handling isn't specified properly
anywhere.
Once we get rid of all the bugs in our shistory, Jesse can generate a few million testcases, and we can package that up and call it a spec.

Problem solved.  :)

The backouts seems reasonable to me.
I can handle the backouts if we go down that road.
Kyle: Make it so!
Attachment #549861 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
We don't want to take this fix, rather we want to backout as comment 49 says. Shall we track that in this bug or in Bug 622315?
(In reply to Christian Legnitto [:LegNeato] from comment #54)
> Kyle: Make it so!

http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=cd5b516e9a39

Marking fixed in 7 by backout.
Might be a good idea to land the test on Aurora too ...
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:8.0a1) Gecko/20110816 Firefox/8.0a1

Is this bug fixed?
I was trying to see if this is fixed for Fx7 since the flag "status-firefox7" is set on "fixed", but I couldn't. On Google + multiple SHEntries are still created and I am still not able to go Forward through the history items.

On FF 7.0b1 I used the following STR:
1. Logged into my gmail accout.
2. In a new tab go to http://plus.google.com/
3. Checked the history entries (right click on the back button)- 4 in my case, sometimes even 5.
4. Clicked on the profile of 3 different users.
5. Go back to the first history entry by clicking on the back button until it becomes grayed out.
6. Click on the Forward button - nothing happens, I'm not taken through the history as it will be expected. 

* Note - same behavior is encountered on the latest Nightly with the difference that in step 3 there are only 2 history entries.
> * Note - same behavior is encountered on the latest Nightly with the difference 
> that in step 3 there are only 2 history entries.

My history also gets stuck when I go back in the latest nightly.  (Two entries is the correct number, however.)  Reopening this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sounds like the remaining issue is bug 680344. If it isn't, we should get a new bug filed (and marked tracking-firefox7 etc.) - too complicated to track multiple occurences in a single bug.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
OS: All → Windows 7
Hardware: All → x86
Resolution: --- → FIXED
Target Milestone: mozilla8 → ---
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → mozilla8
Filed bug 682115 on the continued breakage.
Based on comments 58 through 62, I think QA can call this VERIFIED.
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]
Depends on: 722219
Depends on: 739888
You need to log in before you can comment on or make changes to this bug.