Closed Bug 1375833 Opened 7 years ago Closed 7 years ago

Reload on a frameset page reloads initial rather than current frame

Categories

(Core :: DOM: Navigation, defect, P3)

Product:
Core
Component:
DOM: Navigation
Version:
54 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- wontfix
firefox55 + wontfix
firefox56 + wontfix
firefox57 --- fixed

People

(Reporter: dannyw, Assigned: freesamael)

References

(Regressed 1 open bug,
URL
)

Details

(Keywords: regression)

Attachments

(5 files)

Bug 1375833 - Part 1: Do not clear subframe history on normal reload.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1375833 - Part 2: Set mHistoryID to aSHEntry->DocshellID() in both reload and history navigation.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1375833 - Part 3: Add test case.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1375833 - Part 4.1: Revert test_bug123696.html & update file_bug1326251.html.
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1375833 - Part 4.2 - Fix session store test case.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details 
+++ This bug was initially created as a clone of Bug #181407 +++

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.2) Gecko/20021105 Netscape/7.01
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.2) Gecko/20021105 Netscape/7.01

If you're browsing a page that uses frames doing a reload results in the initial
frameset being reloaded rather than the frame data displayed when the reload is
done.  This also seems to do something wonky to the back/forward history.

Reproducible: Always

Steps to Reproduce:
1. Load the test page
2. Click CAR WASHES link in bottom left frame.
3. Click P21S BODYWORK... link in the bottom left frame
4. Click Reload in the toolbar.

Actual Results:  
I get the initial frameset reloaded.

Expected Results:  
The page that was displayed when Reload was clicked should have been reloaded
(Mozilla does reload the correct frameset)

=====================================================================

OS's in use:  Windows 7 and 8.1
Our company intranet site uses frames. Several of our workstations that have updated to v54 have reported the same issue. When pressing F5 to refresh the data in the main frame, Firefox reloads the "Home"  frame instead.
This now happens all the time.

RightClick -> This Frame -> Reload Frame works ok.
Not many websites use frames anymore, so finding something that isn't our private intranet has been fun!
However I have managed to replicate the problem from this link.

http://www.stumbleupon.com/su/1UjcYE/hbr.org/2016/02/big-companies-should-collaborate-with-startups

Click the menu button and select a different content page. Press F5 and you are taken back to the home page.
Thank you for filing this.  Simple testcase:

  data:text/html,<frameset><frame src="http://example.com"></frameset>

Then click the "More information" link and reload.

Looks like the behavior changed in this one-day checkin range in the 54 cycle: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6dccae211ae5fec6a1c1244b878ce0b93860154f&tochange=fbdfcecf0c774d2221f11aed5a504d5591c774e0

Looking at the docshell diffs in there, I'm guessing this is fallout from bug 1326251; going to verify in a bit.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Bisect says:

The first bad revision is:
changeset:   379958:f3a4d1cafeaa
user:        Samael Wang <freesamael@gmail.com>
date:        Tue Jan 24 14:56:37 2017 +0800
summary:     Bug 1326251 - Part 1: Remove dynamic entries on unloading & evicting bfcache. r=smaug

In this case, there is no bfcache involved, just reloads, and no dynamic entries, right?  Why did the behavior change?
Blocks: 1326251
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox57: --- → affected
tracking-firefox55: --- → ?
tracking-firefox56: --- → ?
Flags: needinfo?(sawang)
Flags: needinfo?(bugs)
Keywords: regression
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #3)
> Bisect says:
> 
> The first bad revision is:
> changeset:   379958:f3a4d1cafeaa
> user:        Samael Wang <freesamael@gmail.com>
> date:        Tue Jan 24 14:56:37 2017 +0800
> summary:     Bug 1326251 - Part 1: Remove dynamic entries on unloading &
> evicting bfcache. r=smaug
> 
> In this case, there is no bfcache involved, just reloads, and no dynamic
> entries, right?  Why did the behavior change?

The original behavior was that only dynamically created entries are dropped on reloading:
http://searchfox.org/mozilla-central/rev/32aac3cc5d7271f9f2804235caea24ccb43016fe/docshell/base/nsDocShell.cpp#11749-11751

The fix for bug 1326251 would make it unreachable, so I was considering to remove that piece of code. Later I realized that all other major browsers Edge/Chrome/Safari drop iframe histories on both history.go(0) and location.reload(), only Firefox doesn't, so I tried to align the behavior with other browsers in the patch instead. (demo http://random-iframe.herokuapp.com/)

However history.go(0) would still not drop the frame history. There was a follow-up bug 1331865 for this.
Flags: needinfo?(sawang)
OK.  So we purposefully changed our frameset-reloading behavior in this situation?
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #5)
> OK.  So we purposefully changed our frameset-reloading behavior in this
> situation?

Yes we did, but I found that I might have been too aggressive about dropping iframe history. I had been confused when trying to figure out different browsers' behaviors before. I re-validated major browsers with these test cases:

http://random-iframe.herokuapp.com/
1. Change iframe.src tho backend
   To verify whether reload retrieves up-to-date content if web author changed iframe.src and re-deployed.

http://freesamael.github.io/gecko/shistory/subframe-nav-reload/iframe.html
2. Change iframe.src tho setTimeout
   To verify if reload may cause history length keep growing. See bug 1326845.
3. Change iframe.src tho script (other than setTimeout)
   To verify how browsers behave when reload after normal iframe location changes.

Firefox
----------------------------------------------------------------------------------------------------------------------
| Test case \ Result \ Operation   | history.go(0) | location.reload() | UI Normal Reload | UI Hard Reload (Ctrl+F5) |
|----------------------------------|---------------------------------------------------------------------------------|
| Change iframe.src tho backend    |     retain    |      reload       |      reload      |     reload               |
| Change iframe.src tho setTimeout |     retain    |      reload       |      reload      |     reload               |
| Change iframe.src tho script     |     retain    |      reload       |      reload      |     reload               |
----------------------------------------------------------------------------------------------------------------------


Edge
------------------------------------------------------------------------------------------------------------
| Test case \ Result \ Operation   | history.go(0) | location.reload() | UI Normal Reload | UI Hard Reload |
|----------------------------------|-----------------------------------------------------------------------|
| Change iframe.src tho backend    |     reload    |      reload       |      reload      |     reload     |
| Change iframe.src tho setTimeout |     retain*   |      reload       |      retain*     |     reload     |
| Change iframe.src tho script     |     retain    |      reload       |      retain      |     reload     |
------------------------------------------------------------------------------------------------------------
* Edge doesn't add history entry when iframe.src is changed tho setTimeout, so it didn't hit bug 1326845.

Chrome
------------------------------------------------------------------------------------------------------------
| Test case \ Result \ Operation   | history.go(0) | location.reload() | UI Normal Reload | UI Hard Reload |
|----------------------------------|-----------------------------------------------------------------------|
| Change iframe.src tho backend    |     reload    |      reload       |      reload      |     reload     |
| Change iframe.src tho setTimeout |     reload*   |      reload*      |      reload*     |     reload*    |
| Change iframe.src tho script     |     reload*   |      reload*      |      reload*     |     reload*    |
------------------------------------------------------------------------------------------------------------
* Althogh Chrome reloads in these cases, history length doesn't change, indicating it didn't drop iframe history.

Safari behaves identical to Chrome.

In general the behavior of Edge looks more preferrable to me. Except that history.go(0) should behave identitcal to location.reload() according to spec https://html.spec.whatwg.org/#dom-history-go

Firefox has been doing well with dynamic iframes, but we should probably re-think how to handle static iframe histories in above cases.
OK, relman can track this for 55 and 56, and if you come up with a patch please feel free to request uplift.
status-firefox54: affected → fix-optional
tracking-firefox55: ? → +
tracking-firefox56: ? → +
The other question is whether "user click on a link" behaves the same as "through script" for purposes of the above tables.
Samael, is this something you're intending to fix for 56? I just noticed this issue is set to P3 so maybe I don't need to track and can leave it to your team's backlog.
status-firefox55: affected → fix-optional
status-firefox56: affected → fix-optional
Flags: needinfo?(sawang)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9)
> this issue is set to P3 so maybe I don't need to track and can leave it to
> your team's backlog.

Let's move it to the backlog :)
Flags: needinfo?(sawang)
Flags: needinfo?(bugs)
It seems like we introduced a UX regression here, no?  And dataloss in some cases, because the reload doesn't reload the thing the user sees... It seems like fixing this should be a higher priority than "backlog".
(In reply to Boris Zbarsky [:bz] from comment #11)
> It seems like we introduced a UX regression here, no?  And dataloss in some
> cases, because the reload doesn't reload the thing the user sees... It seems
> like fixing this should be a higher priority than "backlog".

It is causing UX regression. I'll take the bug.
Assignee: nobody → sawang
(In reply to Boris Zbarsky [:bz] from comment #8)
> The other question is whether "user click on a link" behaves the same as
> "through script" for purposes of the above tables.

Confirmed yes.
(In reply to Samael Wang [:freesamael] from comment #6)
> * Edge doesn't add history entry when iframe.src is changed tho setTimeout, so it didn't hit bug 1326845.

I found it's not that Edge didn't add entries for setTimeout generated frame histories, it's that Edge did some tricks on these entries when pressing UI back / forward buttons.

Test 1:
1. Open a new tab in Edge
2. Goto "http://freesamael.github.io/gecko/shistory/subframe-nav-reload/iframe.html"
   => It should show "history.length=2" at the page bottom
3. Press "change testFrame1.src tho setTimeout"
   => It shows "history.length=3"
4. Press "change testFrame2.src tho setTimeout"
   => It shows "history.length=4"
5. Press UI back button
Result: Instead of performing frame navigation, Edge got back to the new tab page in step 1.

Test 2:
1. Perform step 1~4 in test 1
2. Press "history.go(-1)" button
Result: Frame navigation performed.

Similar tests in http://freesamael.github.io/gecko/shistory/subframe-nav-reload/iframe2.html and http://freesamael.github.io/gecko/shistory/subframe-nav-reload/iframe3.html show same behaviors.

This is an interesting UX but I'm not sure if we can apply the same behavior on Firefox. Edge doesn't show session history list on long pressing / right clicking back button, so the behavior is implicit and is less confusing on Edge.

I also tried IE11; it doesn't provide the same trick as Edge.
(In reply to Samael Wang [:freesamael] from comment #14)
> Edge doesn't show session history list on long pressing / right clicking back button

Oh wait Edge already has this feature back. And the list only contains what UI back / forward buttons would be navigate to.
I'm thinking of implementing hidden history entries for setTimeout case, but also hesitate at doing this - making UI back / forward to skip some entries while not breaking web using history API basically means making nsIWebNavigation.goBack() / goForward() behave differently than history.back() / forward(), which would be quite confusing and complicated when someone look at the code.
Boris, may I have your opinion on comment 16? I may need some suggestions on how we should move this forward.
Flags: needinfo?(bzbarsky)
I'm not sure I quite understand the problem.  What is the "breaking web using history API" issue?  Why do we want the UI to skip entries?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #18)
> I'm not sure I quite understand the problem.  What is the "breaking web
> using history API" issue?  Why do we want the UI to skip entries?

Sorry bad memory of me. To fix this bug, I should not clear frame history on reload, and that would reopen bug 1326845. 

I thought that bug was about bad UX - setTimeout generated frame history entry can cause history list keep growing, so I was thinking of applying Edge's design mentioned in comment 14. Later I recall bug 1326845 is about that in its test case nsIWebNavigation.goBack() would return NS_ERROR_FAILURE, which means back function is broken.

I've identified the root cause - On reloading, a new child docshell would be generated, but docshell would try to apply frame history based on mChildOffset. The problem is that frame entry's docShellID differs from the frame docshell's mHistoryID, so when trying to navigation back, nsSHistory simply can't find any entry for the frame docshell.
Hi Olli, 

I found I've made gecko dropping frame histories too aggressively. Comment 6 is the re-test of all major browsers' behaviors on reloading with subframes. I reversed the behavior of dropping frame histories in a normal reload in the patch, but that would regress bug 1326845, so I also fixed that by setting mHistoryID to entry's docshell ID on reloading.

Would you take a look?
Will review tomorrow at latest.
Comment on attachment 8896923 [details]
Bug 1375833 - Part 1: Do not clear subframe history on normal reload.

https://reviewboard.mozilla.org/r/167498/#review174180

ok, so this is missing the dynamically added docshell stuff bug 1326251 removed. I don't understand yet why, but hopefully other patches explain.
Attachment #8896923 - Flags: review?(bugs) → review+
Comment on attachment 8896924 [details]
Bug 1375833 - Part 2: Set mHistoryID to aSHEntry->DocshellID() in both reload and history navigation.

https://reviewboard.mozilla.org/r/167500/#review174184
Attachment #8896924 - Flags: review?(bugs) → review+
Comment on attachment 8896924 [details]
Bug 1375833 - Part 2: Set mHistoryID to aSHEntry->DocshellID() in both reload and history navigation.

https://reviewboard.mozilla.org/r/167500/#review174186

::: docshell/base/nsDocShell.cpp:10736
(Diff revision 1)
>    // mLSHE should be assigned to aSHEntry, only after Stop() has
>    // been called. But when loading an error page, do not clear the
>    // mLSHE for the real page.
>    if (mLoadType != LOAD_ERROR_PAGE) {
>      SetHistoryEntry(&mLSHE, aSHEntry);
> +    if (aSHEntry) {

Ahaa, this way.

(This all is so super regression risky.)
Comment on attachment 8896925 [details]
Bug 1375833 - Part 3: Add test case.

https://reviewboard.mozilla.org/r/167502/#review174190

::: docshell/test/navigation/file_bug1375833.html:13
(Diff revision 1)
> +  <body onload="test();">
> +    <iframe id="testFrame" src="data:text/html,page1"></iframe>
> +    <script type="application/javascript">
> +    function test() {
> +      let iframe = document.querySelector("#testFrame");
> +      setTimeout(function() { iframe.src = "data:text/html,page1"; }, 0);

Hmm, doesn't this break once we have the new unique origin handling for data: urls.
Perhaps you could make data: pages to post message to parent and then up to parent's opener or something.

I'm talking about the security.data_uri.unique_opaque_origin stuff
Attachment #8896925 - Flags: review?(bugs) → review-
Ah! I forgot that... I would probably just use plain html files instead.
Comment on attachment 8896925 [details]
Bug 1375833 - Part 3: Add test case.

https://reviewboard.mozilla.org/r/167502/#review174436
Attachment #8896925 - Flags: review?(bugs) → review+
And reloading works ok when page has dynamically added (i)frames, right?
(In reply to Olli Pettay [:smaug] from comment #32)
> And reloading works ok when page has dynamically added (i)frames, right?

Dynamic iframe entries are bound to bfcache lifetime so they should still be dropped on reloading, by FirePageHideNotificationInternal (and you remind me that I should probably add some comments about this in patch 1).

There're some test cases need to be updated to reflect the change before landing, I'm still looking at the try result to find all of them. Eventually this part will be updated to verify dynamic / static frame entries on reloading:
http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/docshell/test/navigation/file_bug1326251.html#123-145

However there is one yet-to-fix bug 1364364 which is also related to dynamic iframe history. I'll get into that after landing this bug.
Comment on attachment 8899418 [details]
Bug 1375833 - Part 4.1: Revert test_bug123696.html & update file_bug1326251.html.

https://reviewboard.mozilla.org/r/170690/#review176022
Attachment #8899418 - Flags: review?(bugs) → review+
Comment on attachment 8899419 [details]
Bug 1375833 - Part 4.2 - Fix session store test case.

Try still showing failure on sessionstore/test/browser_707862.js.
Pending the review until fix.
Attachment #8899419 - Flags: review?(mdeboer)
Comment on attachment 8899419 [details]
Bug 1375833 - Part 4.2 - Fix session store test case.

https://reviewboard.mozilla.org/r/170692/#review176816

::: browser/components/sessionstore/test/browser_705597.js:45
(Diff revision 2)
>            let blankState = { windows: [{ tabs: [{ entries: [{ url: "about:blank",
>                                                                triggeringPrincipal_base64}] }]}]};
>            waitForBrowserState(blankState, finish);
>          });
>  
> -        // reload the browser to deprecate the subframes
> +        // force reload the browser to deprecate the subframes

nit: Excellent opportunity to fix these comments to start with a capital letter and end with a dot, like a proper sentence.
Attachment #8899419 - Flags: review?(mdeboer) → review+
Since it's regression ricky I'm making a few more try runs before landing.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7e4839446292
Part 1: Do not clear subframe history on normal reload. r=smaug
https://hg.mozilla.org/integration/autoland/rev/1f8a03070122
Part 2: Set mHistoryID to aSHEntry->DocshellID() in both reload and history navigation. r=smaug
https://hg.mozilla.org/integration/autoland/rev/73747f26f74e
Part 3: Add test case. r=smaug
https://hg.mozilla.org/integration/autoland/rev/e56d79b257ac
Part 4.1: Revert test_bug123696.html & update file_bug1326251.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8a5a636429ea
Part 4.2 - Fix session store test case. r=mikedeboer
Keywords: checkin-needed
Depends on: 1395058
Was this something you wanted to nominate for uplift to Beta or is it OK to let it ride the 57 train?
Flags: needinfo?(sawang)
Since it's considered a regression risky change, I'm willing to make it ride the train without uplifting.
Flags: needinfo?(sawang)
Depends on: 1401522
Regressions: 1751439
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: