Last Comment Bug 673467 - Loading Something Awful forum creates five session history entries
: Loading Something Awful forum creates five session history entries
Status: RESOLVED FIXED
[inbound]
: regression
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 646036
  Show dependency treegraph
 
Reported: 2011-07-22 10:47 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-02-09 07:28 PST (History)
11 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Hacky fix v1. Needs test. (5.45 KB, patch)
2011-07-22 14:06 PDT, Justin Lebar (not reading bugmail)
bzbarsky: feedback-
Details | Diff | Review
Fix? v2 (1.63 KB, patch)
2011-07-26 07:17 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
bzbarsky: feedback+
Details | Diff | Review
Patch v3 (adding test) (4.00 KB, patch)
2011-07-26 09:07 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Test fix v1 (3.59 KB, patch)
2011-07-27 12:04 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Review
Test fix v2 (6.51 KB, patch)
2011-08-02 06:53 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2011-07-22 10:47:17 PDT
Split off from bug 670318 comment 24.  This may or may not be related to that bug, where we observed session history locking up.

STR:

* Disable AdBlock Plus.
* In a new tab, load [1].

Actual results:

* Five session history entries (right-click on the back button to count 'em)

Expected results:

* One session history entry.

[1] http://forums.somethingawful.com/showthread.php?threadid=3397539&userid=0&perpage=30&pagenumber=1
Comment 1 Justin Lebar (not reading bugmail) 2011-07-22 10:49:44 PDT
I'm going to tentatively call this a regression, because it works properly in 3.6.9, but not in 4.0 and beyond.

It could be a bug in the site -- maybe they run different script when they detect FF 4 or later -- but the site works properly in Chrome, which likely also has whatever browser feature they might check for in FF4.

The issue appears to be with child shentries.
Comment 2 Justin Lebar (not reading bugmail) 2011-07-22 10:52:21 PDT
https://plus.google.com also creates more SHEntries than it should, although that site involves the HTML5 history API, so who knows what's going on...
Comment 3 Justin Lebar (not reading bugmail) 2011-07-22 12:34:10 PDT
When we call nsDocShell::AddChildSHEntry for the subframe loads on Something Awful (ads), mLSHE is null, so we end up calling nsSHistory::AddEntry.

On 3.6, mLSHE is non-null at the same point, so we don't call nsSHistory::AddEntry.
Comment 4 Justin Lebar (not reading bugmail) 2011-07-22 14:06:52 PDT
Created attachment 547796 [details] [diff] [review]
Hacky fix v1.  Needs test.
Comment 5 Justin Lebar (not reading bugmail) 2011-07-22 14:10:04 PDT
Comment on attachment 547796 [details] [diff] [review]
Hacky fix v1.  Needs test.

This fixes the multiple-SHEntry behavior on Something Awful, although unfortunately not on G+.  I imagine it breaks something else in a subtle way...

I also am having trouble writing a testcase for this.  It's apparently not just "load an iframe after the root finishes loading".  Maybe I have to try harder to get an about:blank into the iframe first...

Boris, do you think this patch is a reasonable approach?
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-22 20:55:33 PDT
> On 3.6, mLSHE is non-null at the same point,

Any idea why?  When did this change?

The attached patch seems like it will fail a basic test where an <iframe src="http://www.mozilla.org"> is fully loaded, then navigated to about:blank and then navigated to some other site.

Furthermore, right now the ability to go back to the state where the parent page is as it was and the subframe is showing its initial about:blank is generally desired.  I believe that's how it works in other browsers in this simple testcase:

  <!DOCTYPE html>
  <a href="http://www.mozilla.org" target="test">Click me</a>
  <iframe name="test"></iframe>

if you wait for the page to finish loading before clicking the link.  Worth checking that assertion about other browsers, though.
Comment 7 Justin Lebar (not reading bugmail) 2011-07-23 12:19:51 PDT
Firefox, Chrome and Safari create a new SHEntry when you click the link in your testcase.  IE9 and Opera do not.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-23 21:50:36 PDT
Hmm.  I could live with the IE9/Opera behavior there, I guess.  It would certainly simplify things somewhat if loads over initial about:blank never created new shentries.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-23 21:50:55 PDT
On the other hand, it would also look kinda weird to the user.....
Comment 10 Justin Lebar (not reading bugmail) 2011-07-25 12:17:05 PDT
(In reply to comment #9)
> On the other hand, it would also look kinda weird to the user.....

...because the user did something which caused the iframe to navigate away from about:blank and expects to be able to click <back> to go back?  I'm not sure how common the first predicate is -- how often does a user click something which causes an iframe to navigate away from about:blank?
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-25 12:19:47 PDT
> because the user did something which caused the iframe to navigate away from
> about:blank and expects to be able to click <back> to go back?

Yes.

> how often does a user click something which causes an iframe to navigate away
> from about:blank?

Happens to me all the time, but I'm not typical here perhaps....
Comment 12 Justin Lebar (not reading bugmail) 2011-07-25 12:41:18 PDT
>> On 3.6, mLSHE is non-null at the same point,
> Any idea why?  When did this change?

Ah.  In 3.6, we call AddChildEntry on the subframe first.  So mLSHE is non-null because we're navigating while we're loading.  But in tip, we apparently skip the call on the subframe.  mLSHE is non-null on the parent frame, by this point.
Comment 13 Justin Lebar (not reading bugmail) 2011-07-25 15:58:30 PDT
...actually, I think most of what I've said here is wrong, because I haven't been consistent about loading the page by typing into the address bar versus via session restore.
Comment 14 Justin Lebar (not reading bugmail) 2011-07-25 16:06:22 PDT
Okay, this makes much more sense now.

In nsDocShell::AddToSessionHistory, we check whether mLoadType has LOAD_FLAGS_REPLACE_HISTORY.  If mLoadType has REPLACE_HISTORY, we skip DoAddChildSHEntry.

In 3.6, mLoadType on the iframe's initial load has REPLACE_HISTORY, while in 4.0, it doesn't.  Now I need to figure out why...

Sorry for the confusion before; I should have known better.
Comment 15 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-25 16:44:53 PDT
This sounds a lot like bug 636066, which seems to have the same broken time frame (started in 4). I haven't looked at it recently, but might be that we can just dupe that here.
Comment 16 Justin Lebar (not reading bugmail) 2011-07-25 17:26:35 PDT
This sounds like exactly what's going on in that bug, but that bug also talks about its effect on sessionstore.js's size, and the resulting impact on speed.  So let's let this one be about the illegitimate shentries, and let that one be about the large file and slowness.

Bug 580819 seems likely to have caused this bug.
Comment 17 Justin Lebar (not reading bugmail) 2011-07-25 17:49:42 PDT
Sadly, undoing bug 580819 doesn't fix this.
Comment 18 Justin Lebar (not reading bugmail) 2011-07-25 18:14:40 PDT
In 3.6, LOAD_NORMAL_REPLACE gets set at [1].  That doesn't happen in 4.0, because the busy flags aren't right.

[1] http://hg.mozilla.org/mozilla-central/file/504a1a927d39/docshell/base/nsDocShell.cpp#l1341
Comment 19 Justin Lebar (not reading bugmail) 2011-07-25 18:19:05 PDT
Er, not because of the busy flags.  Because of the change in bug 462076, which caused shEntry to be null.  http://hg.mozilla.org/mozilla-central/file/504a1a927d39/docshell/base/nsDocShell.cpp#l1287
Comment 20 Justin Lebar (not reading bugmail) 2011-07-25 19:12:45 PDT
Actually -- and I think this is the last correction, because there aren't any other available permutations -- shEntry is null *and* the busy flags indicate that we're not busy.  In 3.6, shEntry is non-null, and parentBusy is non-zero.
Comment 21 Justin Lebar (not reading bugmail) 2011-07-26 07:17:42 PDT
Created attachment 548440 [details] [diff] [review]
Fix? v2

I think I may finally understand this.

Bug 462076 changed two things.  First, it says, don't bother looking up the
shentry that our parent has for this iframe if the iframe was just added,
because what we find isn't going to be right anyway.  It also says that
mChildOffset is -1 for docshells corresponding to dynamically-inserted iframes.

So in nsDocShell::LoadURI, we'll set the load type to REPLACE if either this
docshell or its parent is busy (in this case, the parent is busy) and if we
have an shEntry for the child.  But we don't have an shEntry for the child,
because

 a) we decided not to look it up (because it'd be wrong anyway), and
 b) even if we did look it up, mChildOffset is -1, which doesn't correspond to
    an shEntry.

This patch just takes out the requirement that shEntry is non-null.  I hate
touching this, because you never know what you're going to break.  But the good
news is that we're going to set shEntry to null *anyway*, so the only real
change is with respect to the load flags.
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-26 07:23:59 PDT
_That_ change makes a lot of sense to me.
Comment 23 Justin Lebar (not reading bugmail) 2011-07-26 07:24:30 PDT
Comment on attachment 548440 [details] [diff] [review]
Fix? v2

I'm going to write a test, but in the meantime, bz, does this look remotely sensible?  I have no idea why the shEntry check is there to begin with...
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-26 07:50:50 PDT
Comment on attachment 548440 [details] [diff] [review]
Fix? v2

I think this is the right change.

I did look up the blame for this.  Looks like CVS revision 1.428 of nsDocShell.cpp added this code block with the |shEntry| check already in place, but with LOAD_BYPASS_HISTORY for the load type.  That was later changed to LOAD_NORMAL_REPLACE in rev 1.513.

No obvious reason why the code is the way it is, and I think it's wrong as is.
Comment 25 Justin Lebar (not reading bugmail) 2011-07-26 09:07:23 PDT
Created attachment 548481 [details] [diff] [review]
Patch v3 (adding test)

bz, do you want to look at the test?
Comment 26 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-26 09:49:21 PDT
Comment on attachment 548481 [details] [diff] [review]
Patch v3 (adding test)

Test looks good to me.  Thanks!
Comment 27 Justin Lebar (not reading bugmail) 2011-07-26 10:09:40 PDT
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/f57324fe25c9
Comment 28 Matt Brubeck (:mbrubeck) 2011-07-26 15:23:58 PDT
Backed out because of mochitest failures:
http://hg.mozilla.org/integration/mozilla-inbound/rev/53401a5a9a30
Comment 29 Justin Lebar (not reading bugmail) 2011-07-26 15:45:45 PDT
25952 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug426646.html | Test timed out.
50630 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for select - select should be a [object HTMLSelectElement]
50631 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for textarea - textarea should be a [object HTMLTextAreaElement]
50632 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for text - text should be a [object HTMLInputElement]
50633 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for password - password should be a [object HTMLInputElement]
50634 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for checkbox - checkbox should be a [object HTMLInputElement]
50635 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for radio - radio should be a [object HTMLInputElement]
50636 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for image - image should be a [object HTMLInputElement]
50637 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for submit - submit should be a [object HTMLInputElement]
50638 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for reset - reset should be a [object HTMLInputElement]
50639 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for button input - button input should be a [object HTMLInputElement]
50640 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for hidden - hidden should be a [object HTMLInputElement]
50641 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for file - file should be a [object HTMLInputElement]
50642 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for submit button - submit button should be a [object HTMLButtonElement]
50643 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for reset button - reset button should be a [object HTMLButtonElement]
50644 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_bug277724.html | Check for button - button should be a [object HTMLButtonElement]
Comment 30 Mike Shaver (:shaver -- probably not reading bugmail closely) 2011-07-27 06:16:23 PDT
(FWIW, in bug 673130 where I lose the use of my back button, I couldn't reproduce the behaviour with Aurora.  I'll mozregression it today to see if I can narrow it down further.)
Comment 31 Justin Lebar (not reading bugmail) 2011-07-27 12:04:25 PDT
Created attachment 548860 [details] [diff] [review]
Test fix v1
Comment 32 Justin Lebar (not reading bugmail) 2011-07-27 12:06:40 PDT
Comment on attachment 548860 [details] [diff] [review]
Test fix v1

This test fails because it navigates from within the iframes' onload handlers.  The test expects this to create a new SHEntry, but it shouldn't -- that's this bug!
Comment 33 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-07-31 09:35:30 PDT
Comment on attachment 548860 [details] [diff] [review]
Test fix v1

The nowhere-properly-defined session history handling is just great :/
Comment 35 Dão Gottwald [:dao] 2011-07-31 12:42:01 PDT
backed out for test_bug277724.html failures
Comment 36 Dão Gottwald [:dao] 2011-07-31 12:42:51 PDT
Comment on attachment 548481 [details] [diff] [review]
Patch v3 (adding test)

>+      let shistory = tabBrowser.contentWindow
>+                      .QueryInterface(Ci.nsIInterfaceRequestor)
>+                      .getInterface(Ci.nsIWebNavigation)
>+                      .sessionHistory;

the browser itself has a "sessionHistory" getter
Comment 37 Justin Lebar (not reading bugmail) 2011-08-01 08:50:29 PDT
Bah, I was so convinced those bug277724 failures were caused by the bug426646 timeout, I didn't even think to check after I fixed that one.
Comment 38 Justin Lebar (not reading bugmail) 2011-08-02 06:53:25 PDT
Created attachment 550066 [details] [diff] [review]
Test fix v2

With this fix to bug 277724, the patch is green on try.
Comment 39 Justin Lebar (not reading bugmail) 2011-08-02 06:56:14 PDT
Comment on attachment 550066 [details] [diff] [review]
Test fix v2

...scratch that; it appears that this may tickle a randomorange into happening much more often, or something.
Comment 40 Justin Lebar (not reading bugmail) 2011-08-02 08:01:03 PDT
New try push; there may have been something wrong with the revision I pushed from.  http://tbpl.mozilla.org/?tree=Try&rev=3feac2baf68e
Comment 41 Justin Lebar (not reading bugmail) 2011-08-03 08:38:53 PDT
Okay.  There's absolutely no orange in the (macos) try push I did.  Smaug, can you review this new test change?
Comment 42 Justin Lebar (not reading bugmail) 2011-08-03 09:41:38 PDT
Inbound: http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9d673a0033f9

Note You need to log in before you can comment on or make changes to this bug.