1207283 - nsDocShell::LoadURI makes the wrong decision for using the parent's session history object to override the URL of an iframe

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

STR (by ehsan):

1) View the attached testcase
2) The page's iframe should say "sucker"
3) Reload the page

ER:

The iframe should not say "sucker" after the reload.

AR:

The iframe says "sucker".


It looks like we're falling into this branch: https://dxr.mozilla.org/mozilla-central/rev/a1ccea59e254a88f7bb44b0ad8a58b77b7eca339/docshell/base/nsDocShell.cpp#1394

Which is causing us to retrieve the data URI from the shEntry to set the iframe's URI to.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: testcase.html

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8664406 [details]
testcase.html

Strangely, this bug doesn't appear over HTTPS. :/

How about this: http://people.mozilla.org/~mconley2/moz/weird-testcase.html

Comment 3

[(no longer active)]

Boris, is this behavior intentional?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, it is.  When reloading, we reload the current URI of the iframe, not the original URI.

To see why, consider these steps:

1)  Load page A that frames page B.
2)  Click a link in B that navigates the frame to C.
3)  Reload.

The user's expectation at this point is that they reload whatever they're seeing, so A at toplevel and C in the subframe.

I have no clue why it's not working with bugzilla, honestly; I've have to step through and debug it.  I'd expect it to work exactly the same way there.

Comment 5

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #4)
> Yes, it is.  When reloading, we reload the current URI of the iframe, not
> the original URI.
> 
> To see why, consider these steps:
> 
> 1)  Load page A that frames page B.
> 2)  Click a link in B that navigates the frame to C.
> 3)  Reload.
> 
> The user's expectation at this point is that they reload whatever they're
> seeing, so A at toplevel and C in the subframe.

I agree.  But you're noting that what this test case does is different than the above scenario, right?  Namely in that the user hasn't navigated anything.  It is the parent page that has navigated the iframe dynamically.

> I have no clue why it's not working with bugzilla, honestly; I've have to
> step through and debug it.  I'd expect it to work exactly the same way there.

That seems to be an https difference.  For example, <http://people.mozilla.org/~mconley2/moz/weird-testcase.html> reproduces, but <https://people.mozilla.org/~mconley2/moz/weird-testcase.html> doesn't.  But note that this difference is not what this bug is about.

Comment 6

[(no longer active)]

(Also, according to our tests, we're the only UA that does this, the test case works fine in all other browsers.  We did not test Edge but tested IE11.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> It is the parent page that has navigated the iframe dynamically.

Sure.  Just like it could be the parent page doing a location set on the iframe in response to a user click...

> the test case works fine in all other browsers

How do other browsers handle the situation I described in comment 4?  Last I checked at least some of them just blew away subframe state and reloaded the toplevel, then did normal loads from there instead of trying to preserve subframe state.

Comment 8

[Olivier Yiptong [:oyiptong]]

(In reply to Boris Zbarsky [:bz] from comment #7)

> How do other browsers handle the situation I described in comment 4?  Last I
> checked at least some of them just blew away subframe state and reloaded the
> toplevel, then did normal loads from there instead of trying to preserve
> subframe state.

The key difference is that the other browsers reload the subframe to whatever the src location is set to.
In this case, since the src is set dynamically, the parent page's state stores the iframe without a src attribute.

Currently, in Firefox, this iframe loads with its previous content, rather than with content following the src declaration.

The expectation is that the iframe would load about:blank if no src is set.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> The other browsers reload the subframe to whatever the src location is set to.

Yes, which is totally bogus in the scenario I describe in comment 4, no?

Comment 10

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > The other browsers reload the subframe to whatever the src location is set to.
> 
> Yes, which is totally bogus in the scenario I describe in comment 4, no?

Sure, but we can detect the two cases.  I'm not sure why comment 4 precludes fixing comment 0.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So one thing that's been proposed before (and I can find the relevant bug if that would be useful) is to remember in session history whether the url in the frame matched the src and if so on reload to load the src (whaver that is at that point) instead of whatever used to be in the frame.  That sort of handles sites that dynamically write out a different src each time.

Comment 12

[Olivier Yiptong [:oyiptong]]

That would be very useful, Boris. We're in the situation where implementing this proposal would be very helpful.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm.  I did some looking around, and I can't find the bug that was suggested in.  In any case, the actual work can just happen in this bug.

Comment 14

[(no longer active)]

That makes sense to me.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Hey oyiptong - does this need to block any of that newtab work?

Comment 16

[Olivier Yiptong [:oyiptong]]

We worked around it by waiting for a message to be sent from inside the iframe which only runs on document ready. So, we're good.