Closed Bug 673467 Opened 13 years ago Closed 13 years ago

Loading Something Awful forum creates five session history entries

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

(Keywords: regression, Whiteboard: [inbound])

Attachments

(2 files, 3 obsolete files)

Patch v3 (adding test)
4.00 KB, patch
Details | Diff | Splinter Review
Test fix v2
6.51 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
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
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
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...
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.
Attached patch Hacky fix v1. Needs test. (obsolete) — — Splinter Review 

    
    

    
  
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.

    
    

    
  
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.....

    
    

    
  
(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....

    
    

    
  
>> 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.

    
    

    
  
...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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
Sadly, undoing bug 580819 doesn't fix this.

    
    

    
  
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

    
    

    
  
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

    
    

    
  
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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix? v2 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

    
  

        Attachment #547796 -
        Attachment is obsolete: true

    
    

    
  
_That_ change makes a lot of sense to me.

    
    

    
  
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+

    
    

    
  


  
bz, do you want to look at the test?

    
  

        Attachment #548440 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 548481 [details] [diff] [review]
Patch v3 (adding test)

Test looks good to me.  Thanks!

    
    

    
  
Backed out because of mochitest failures:
http://hg.mozilla.org/integration/mozilla-inbound/rev/53401a5a9a30
Whiteboard: [inbound]

    
    

    
  
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.)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test fix v1 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #548860 -
        Flags: review?(Olli.Pettay)

    
  

        Attachment #548481 -
        Attachment is obsolete: true

    
    

    
  
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!

    
  

        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+

    
  
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

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test fix v2
        — 
          — Splinter Review
      

    


  
With this fix to bug 277724, the patch is green on try.

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

    
  

        Attachment #548481 -
        Attachment is obsolete: true

    
  

        Attachment #548860 -
        Attachment is obsolete: true

    
    

    
  
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)

    
    

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

    
  

        Attachment #548481 -
        Attachment is obsolete: false

    
    

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

    
  

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

        Attachment #550066 -
        Flags: review?(Olli.Pettay) → review+

    
  
Whiteboard: [inbound]

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/eee594b2f591
http://hg.mozilla.org/mozilla-central/rev/9d673a0033f9
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

    
  
Flags: in-testsuite+

    
  
Blocks: 646036

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: