Open Bug 718908 Opened 12 years ago Updated 2 years ago

Only one onlocationchange event fired when navigating two iframes

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

Attachments

(2 files)

Testcase (put a breakpoint or printf in nsDocLoader::FireOnLocationChange)
382 bytes, text/html
Details
Patch v1
1.17 KB, patch
Details | Diff | Splinter Review 
I doubt this has a practical effect currently, but it blocks correct execution of bug 710231.  I suspect this will not be the last "nsIWebProgressListener events are wrong" bug I'll face, sadly.

When you run the attached testcase, only one locationchange event is fired, even though both iframes navigate.  The locationchange event is for the first iframe navigated.

If you yield to the event loop before setting the second iframe's location, then you get two events, as expected.
If I load the testcase, put a breakpoint in nsDocLoader::FireOnLocationChange, and click the button, I see the breakpoint hit 4 times, as expected, in a current Mac m-c debug nightly....
I see four occurrences as well (so the bug summary is off the mark), but the URIs I see are wrong.

With the following code added to FireOnLocationChange,

  nsCString spec;
  aUri->GetSpec(spec);
  printf("[%p] FireOnLocationChange %s\n", this, spec.get());

I get

[0x1176a4000] FireOnLocationChange http://www.mozilla.org/en-US/firefox/fx/
[0x138a7cc00] FireOnLocationChange http://www.mozilla.org/en-US/firefox/fx/
[0x10ca6a000] FireOnLocationChange http://www.mozilla.org/en-US/firefox/fx/
[0x1003c7470] FireOnLocationChange http://www.mozilla.org/en-US/firefox/fx/.

When I set the iframe's location to mozilla.org first, I get

[0x116cb5c00] FireOnLocationChange http://www.mozilla.org/
[0x10d325800] FireOnLocationChange http://www.mozilla.org/
[0x10ca6a000] FireOnLocationChange http://www.mozilla.org/
[0x1003c7470] FireOnLocationChange http://www.mozilla.org/
(Note that mozilla.com redirects to http://www.mozilla.org/en-US/firefox/fx/.)
Oh, hrm.  So the 4 OnLocationChanges are the subframe, the content area, the browser window, and the root docloader.

So you're right, no notification for the other subframe.  The reason is this, apparently:

1824        if (!isSubFrame && !isRoot) {
1825          /* 
1826           * We don't want to send OnLocationChange notifications when
1827           * a subframe is being loaded for the first time, while
1828           * visiting a frameset page
1829           */
1830          return false; 
1831        }

So the issue is that for some reason GetIsSubFrame on mLSHE returned false, when it should return true.  Why?
Component: General → Document Navigation
QA Contact: general → docshell
> So the issue is that for some reason GetIsSubFrame on mLSHE returned false, when it should return 
> true.  Why?

The key is the call to AddToSessionHistory in OnNewURI.  AddToSessionHistory eventually calls CloneAndReplaceChild, which sets IsSubFrame.

Apparently AddToSessionHistory is not being called for the second iframe in the testcase.

#0  nsSHEntry::SetIsSubFrame (this=0x318c0d0, aFlag=true) at ../../../../src/docshell/shistory/src/nsSHEntry.cpp:317
#1  0x00007ffff46cadc6 in nsDocShell::CloneAndReplaceChild (aEntry=0x174f820, aShell=0x18015c0, aEntryIndex=1, aData=0x7fffffffa450) at ../../../src/docshell/base/nsDocShell.cpp:10202
#2  0x00007ffff46caaa1 in nsDocShell::WalkHistoryEntries (aRootEntry=0x7fffd886ba40, aRootShell=0x2d50cc0, aCallback=0x7ffff46cabc8 <nsDocShell::CloneAndReplaceChild(nsISHEntry*, nsDocShell*, int, void*)>, aData=0x7fffffffa450)
    at ../../../src/docshell/base/nsDocShell.cpp:10147
#3  0x00007ffff46cae38 in nsDocShell::CloneAndReplaceChild (aEntry=0x7fffd886ba40, aShell=0x2d50cc0, aEntryIndex=0, aData=0x7fffffffa520) at ../../../src/docshell/base/nsDocShell.cpp:10209
#4  0x00007ffff46cb066 in nsDocShell::CloneAndReplace (aSrcEntry=0x7fffd886ba40, aSrcShell=0x2d50cc0, aCloneID=12, aReplaceEntry=0x9e7520, aCloneChildren=false, aResultEntry=0x7fffffffa610) at ../../../src/docshell/base/nsDocShell.cpp:10236
#5  0x00007ffff46aebea in nsDocShell::AddChildSHEntry (this=0x2d50cc0, aCloneRef=0x2c3cfa0, aNewEntry=0x9e7520, aChildOffset=0, loadType=134217729, aCloneChildren=false) at ../../../src/docshell/base/nsDocShell.cpp:3437
#6  0x00007ffff46aef98 in nsDocShell::DoAddChildSHEntry (this=0x24edaf0, aNewEntry=0x9e7520, aChildOffset=0, aCloneChildren=false) at ../../../src/docshell/base/nsDocShell.cpp:3483
#7  0x00007ffff46c9cf2 in nsDocShell::AddToSessionHistory (this=0x24edaf0, aURI=0x31df910, aChannel=0x3b6de78, aOwner=0x0, aCloneChildren=false, aNewEntry=0x24edd80) at ../../../src/docshell/base/nsDocShell.cpp:9973
#8  0x00007ffff46c6fa1 in nsDocShell::OnNewURI (this=0x24edaf0, aURI=0x31df910, aChannel=0x3b6de78, aOwner=0x0, aLoadType=134217729, aFireOnLocationChange=true, aAddToGlobalHistory=false, aCloneSHChildren=false) at ../../../src/docshell/base/nsDocShell.cpp:9391
So AddToSessionHistory is called for both frames, but for the errant frame, the following check fails:

        // This is a subframe.
        if (!mOSHE || !LOAD_TYPE_HAS_FLAGS(mLoadType,
                                           LOAD_FLAGS_REPLACE_HISTORY))
            rv = DoAddChildSHEntry(entry, mChildOffset, aCloneChildren);

mOSHE has URI "http://mozilla.org" (?), and mLoadType is 0x800001, which has LOAD_FLAGS_REPLACE_HISTORY.
That part is correct, possibly (in that we don't want to create two entries in the toplevel session history for the case of two subframes loading in parallel).

But then we should flag the entry as a subframe entry in the else, I'd think.
Whereas when we load the iframe which does get a locationchange event, its load type is 0x8000001 (one more 0 than before), which corresponds to LOAD_STOP_CONTENT.
> But then we should flag the entry as a subframe entry in the else, I'd think.

Which |else|?  There is no else to the if statement in comment 6.
> Which |else|?  There is no else to the if statement in comment 6.

Precisely!  If the load type is what it should be, we should add such an else.
Oh, you mean the |else| containing the if statement?  That is

    if (root == static_cast<nsIDocShellTreeItem *>(this) && mSessionHistory) {
        // ...
    }
    else {  
        // This is a subframe.
        if (!mOSHE || !LOAD_TYPE_HAS_FLAGS(mLoadType,
                                           LOAD_FLAGS_REPLACE_HISTORY))
            rv = DoAddChildSHEntry(entry, mChildOffset, aCloneChildren);

        // ** Ensure here that entry is marked as a child frame.
    }

That would make sense to me, inasmuch as anything here makes sense.
To be more clear, it seems like the "this is a subframe" case should reliably flag the shentry for the subframe as a subframe entry.
> If the load type is what it should be, we should add such an else.

For which load types should we not SetIsSubFrame(true)?
No, I meant that if the mLoadType above is actually correct, then we need an else.

If the mLoadType is wrong, we should fix that.  And maybe stil add the else.
> That part is correct, possibly (in that we don't want to create two entries in the toplevel session 
> history for the case of two subframes loading in parallel).

It sounds like the loadtype is correct, since we don't want the second load to create a new SHEntry.
Attached patch Patch v1 — — Splinter Review 

        Attachment #589743 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 589743 [details] [diff] [review]
Patch v1

My preference would be to remove the SetIsSubFrame(true) call hit via DoAddChildSHEntry, but it's hard to imagine an amount of testing which would convince me that this is safe.

    
    

    
  
Hmm.  So I think docshell relies on not setting isSubframe to true for the initial load in a frame.  Will that still be true with that change?

    
    

    
  
> Will that still be true with that change?

Probably not.

Can we write a test for this?  I guess the relevant code is what you quoted in comment 4.  Check for the right number of onlocationchange events when loading into a subframe?

    
    

    
  
Yeah, I guess.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #19)
> Hmm.  So I think docshell relies on not setting isSubframe to true for the
> initial load in a frame.  Will that still be true with that change?

Why do we even want to suppress the OnLocationChange notification for the initial load in a frame?  This complicates my life in bug 710231 (there, I would really like to be notified on the initial load), and in general, I don't see why it's desirable.

    
    

    
  
I'm not actually sure.  I assume that some code somewhere assumed that onLocationChange involved an actual new entry in the session history or something and docshell was hacked to sort of ensure that.  But it's hard to say whether anything still depends on that without auditing all onLocationChange listeners....  That might not be that hard, though.

    
    

    
  
> I assume that some code somewhere assumed that onLocationChange involved an actual new entry in the 
> session history or something and docshell was hacked to sort of ensure that.

Well, we're breaking that first assumption anyway (because the second iframe will now send a locationchange, but there's only one shentry for both locationchanges), so it sounds like I need to check.

    
    

    
  
Justin, are you still waiting on me here?

    
    

    
  
No.  This still happens, but it's not blocking me or the Gaia team at the moment, and it's not clear that this bug will be hit in the final architecture of <iframe mozbrowser>.  So I'm giving it a rest until it's proven relevant.

    
  

        Attachment #589743 -
        Flags: review?(bzbarsky)

    
  
Severity: normal → S3

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: