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)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla8
People
(Reporter: jaws, Assigned: ttaubert)
References
Details
(Keywords: regression, Whiteboard: [qa!])
Attachments
(3 files, 2 obsolete files)
64.42 KB,
video/mp4
|
Details | |
16.26 KB,
text/plain
|
Details | |
3.86 KB,
patch
|
smaug
:
review+
christian
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
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).
Comment 1•13 years ago
|
||
> 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
Comment 2•13 years ago
|
||
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....
Reporter | ||
Comment 3•13 years ago
|
||
(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.
Comment 4•13 years ago
|
||
Steps to reproduce would be great.
Comment 5•13 years ago
|
||
(I don't have any software on this laptop which could play mp4)
Reporter | ||
Comment 6•13 years ago
|
||
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?
Reporter | ||
Comment 7•13 years ago
|
||
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.
Comment 10•13 years ago
|
||
> 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...
Comment 11•13 years ago
|
||
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.
Comment 12•13 years ago
|
||
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...
Updated•13 years ago
|
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
Comment 13•13 years ago
|
||
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.
Comment 14•13 years ago
|
||
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.
Comment 15•13 years ago
|
||
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!")
Comment 16•13 years ago
|
||
But anyway, I wonder if the session history is cleaning up itself too aggressively.
Comment 17•13 years ago
|
||
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.
Comment 18•13 years ago
|
||
Comment 19•13 years ago
|
||
nsSHistory.cpp, line 867 is interesting.
Comment 20•13 years ago
|
||
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.
Comment 22•13 years ago
|
||
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?
Comment 23•13 years ago
|
||
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).
Comment 24•13 years ago
|
||
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.
Comment 25•13 years ago
|
||
Confirmed the same behavior on G+ and Something Awful on Aurora and Beta. At least this isn't a recent regression...
Comment 26•13 years ago
|
||
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.
Comment 27•13 years ago
|
||
Confirmed that Something Awful isn't calling push/replaceState, so something else is going on here.
Comment 28•13 years ago
|
||
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. :-)
Comment 30•13 years ago
|
||
> 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.
Comment 31•13 years ago
|
||
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.
Comment 32•13 years ago
|
||
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
Updated•13 years ago
|
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
Comment 33•13 years ago
|
||
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!
Comment 34•13 years ago
|
||
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.
Comment 35•13 years ago
|
||
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.
Comment 36•13 years ago
|
||
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.
Comment 37•13 years ago
|
||
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.
Updated•13 years ago
|
tracking-firefox8:
--- → +
Comment 38•13 years ago
|
||
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
Assignee | ||
Comment 39•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #549805 -
Flags: review?(bzbarsky) → review+
Comment 40•13 years ago
|
||
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?
Assignee | ||
Comment 41•13 years ago
|
||
(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
Assignee | ||
Comment 42•13 years ago
|
||
Test added and ensured that it fails without the patch applied.
Attachment #549805 -
Attachment is obsolete: true
Attachment #549861 -
Flags: review?(Olli.Pettay)
Updated•13 years ago
|
Attachment #549861 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 43•13 years ago
|
||
http://hg.mozilla.org/integration/fx-team/rev/968f8f50e428
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Comment 44•13 years ago
|
||
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
Assignee | ||
Comment 45•13 years ago
|
||
STR is reproducible on Aurora, not in Beta.
tracking-firefox7:
--- → ?
Comment 46•13 years ago
|
||
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?
Keywords: regression,
regressionwindow-wanted
Comment 47•13 years ago
|
||
Justin / Tim / QA: Can we find the bug that regressed this in aurora (or exposed a latent issue)? Thanks!
Assignee | ||
Comment 48•13 years ago
|
||
(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
Assignee | ||
Updated•13 years ago
|
Keywords: regressionwindow-wanted
Comment 49•13 years ago
|
||
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.
Comment 50•13 years ago
|
||
> 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...
Comment 51•13 years ago
|
||
(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.
Comment 52•13 years ago
|
||
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.
Comment 54•13 years ago
|
||
Kyle: Make it so!
Attachment #549861 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Comment 55•13 years ago
|
||
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.
status-firefox7:
--- → fixed
Might be a good idea to land the test on Aurora too ...
status-firefox8:
--- → fixed
Comment 58•13 years ago
|
||
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.
Comment 59•13 years ago
|
||
> * 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 → ---
Comment 60•13 years ago
|
||
This could be bug 680344.
Comment 61•13 years ago
|
||
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 ago → 13 years ago
OS: All → Windows 7
Hardware: All → x86
Resolution: --- → FIXED
Target Milestone: mozilla8 → ---
Updated•13 years ago
|
OS: Windows 7 → All
Hardware: x86 → All
Target Milestone: --- → mozilla8
Comment 62•13 years ago
|
||
Filed bug 682115 on the continued breakage.
Comment 63•13 years ago
|
||
Based on comments 58 through 62, I think QA can call this VERIFIED.
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]
Comment hidden (obsolete) |
You need to log in
before you can comment on or make changes to this bug.
Description
•