Only one onlocationchange event fired when navigating two iframes

NEW
Unassigned

Status

()

Core
Document Navigation
6 years ago
6 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 589376 [details]
Testcase (put a breakpoint or printf in nsDocLoader::FireOnLocationChange)

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

Comment 2

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

Comment 3

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

Updated

6 years ago
Component: General → Document Navigation
QA Contact: general → docshell
(Reporter)

Comment 5

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

Comment 6

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

Comment 8

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

Comment 9

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

Comment 11

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

Comment 13

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

Comment 15

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

Comment 16

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=6e88bce984c4
(Reporter)

Comment 17

6 years ago
Created attachment 589743 [details] [diff] [review]
Patch v1
Attachment #589743 - Flags: review?(bzbarsky)
(Reporter)

Comment 18

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

Comment 20

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

Comment 22

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

Comment 24

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

Comment 26

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

Updated

6 years ago
Attachment #589743 - Flags: review?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.