The default bug view has changed. See this FAQ.

Loading Something Awful forum creates five session history entries

RESOLVED FIXED in mozilla8

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Unassigned)

Tracking

({regression})

unspecified
mozilla8
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
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.
Keywords: regression
(Reporter)

Comment 2

6 years ago
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...
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Created attachment 547796 [details] [diff] [review]
Hacky fix v1.  Needs test.
(Reporter)

Comment 5

6 years ago
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?
Attachment #547796 - Flags: feedback?(bzbarsky)
> 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.
(Reporter)

Comment 7

6 years ago
Firefox, Chrome and Safari create a new SHEntry when you click the link in your testcase.  IE9 and Opera do not.
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.
On the other hand, it would also look kinda weird to the user.....
(Reporter)

Comment 10

6 years ago
(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?
> 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....
(Reporter)

Comment 12

6 years ago
>> 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.
(Reporter)

Comment 13

6 years ago
...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.
(Reporter)

Comment 14

6 years ago
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.
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.
(Reporter)

Comment 16

6 years ago
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.
(Reporter)

Comment 17

6 years ago
Sadly, undoing bug 580819 doesn't fix this.
(Reporter)

Comment 18

6 years ago
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
(Reporter)

Comment 19

6 years ago
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
(Reporter)

Comment 20

6 years ago
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.
Attachment #547796 - Flags: feedback?(bzbarsky) → feedback-
(Reporter)

Comment 21

6 years ago
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.
(Reporter)

Updated

6 years ago
Attachment #547796 - Attachment is obsolete: true
_That_ change makes a lot of sense to me.
(Reporter)

Comment 23

6 years ago
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...
Attachment #548440 - Attachment description: Fix? v1 → Fix? v2
Attachment #548440 - Flags: feedback?(bzbarsky)
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.
Attachment #548440 - Flags: review+
Attachment #548440 - Flags: feedback?(bzbarsky)
Attachment #548440 - Flags: feedback+
(Reporter)

Comment 25

6 years ago
Created attachment 548481 [details] [diff] [review]
Patch v3 (adding test)

bz, do you want to look at the test?
(Reporter)

Updated

6 years ago
Attachment #548440 - Attachment is obsolete: true
Comment on attachment 548481 [details] [diff] [review]
Patch v3 (adding test)

Test looks good to me.  Thanks!
(Reporter)

Comment 27

6 years ago
Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/f57324fe25c9
Whiteboard: [inbound]
Backed out because of mochitest failures:
http://hg.mozilla.org/integration/mozilla-inbound/rev/53401a5a9a30
Whiteboard: [inbound]
(Reporter)

Comment 29

6 years ago
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]
(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.)
(Reporter)

Comment 31

6 years ago
Created attachment 548860 [details] [diff] [review]
Test fix v1
Attachment #548860 - Flags: review?(Olli.Pettay)
(Reporter)

Updated

6 years ago
Attachment #548481 - Attachment is obsolete: true
(Reporter)

Comment 32

6 years ago
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!
(Reporter)

Updated

6 years ago
Attachment #548481 - Attachment is obsolete: false
Comment on attachment 548860 [details] [diff] [review]
Test fix v1

The nowhere-properly-defined session history handling is just great :/
Attachment #548860 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Comment 34

6 years ago
Inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/7ad075ec023b
http://hg.mozilla.org/integration/mozilla-inbound/rev/2878f506066a
(Reporter)

Updated

6 years ago
Whiteboard: [inbound]
backed out for test_bug277724.html failures
Whiteboard: [inbound]
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
(Reporter)

Comment 37

6 years ago
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.
(Reporter)

Comment 38

6 years ago
Created attachment 550066 [details] [diff] [review]
Test fix v2

With this fix to bug 277724, the patch is green on try.
Attachment #550066 - Flags: review?(Olli.Pettay)
(Reporter)

Updated

6 years ago
Attachment #548481 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #548860 - Attachment is obsolete: true
(Reporter)

Comment 39

6 years ago
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.
Attachment #550066 - Flags: review?(Olli.Pettay)
(Reporter)

Comment 40

6 years ago
New try push; there may have been something wrong with the revision I pushed from.  http://tbpl.mozilla.org/?tree=Try&rev=3feac2baf68e
(Reporter)

Updated

6 years ago
Attachment #548481 - Attachment is obsolete: false
(Reporter)

Comment 41

6 years ago
Okay.  There's absolutely no orange in the (macos) try push I did.  Smaug, can you review this new test change?
(Reporter)

Updated

6 years ago
Attachment #550066 - Flags: review?(Olli.Pettay)

Updated

6 years ago
Attachment #550066 - Flags: review?(Olli.Pettay) → review+
(Reporter)

Comment 42

6 years ago
Inbound: http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9d673a0033f9
(Reporter)

Updated

6 years ago
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/eee594b2f591
http://hg.mozilla.org/mozilla-central/rev/9d673a0033f9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

Updated

6 years ago
Flags: in-testsuite+

Updated

5 years ago
Blocks: 646036
You need to log in before you can comment on or make changes to this bug.