2023917 - browsingContext.reload doesn't reset location of navigated iframe

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, defect, P3)

Description

[Holger Benl [:hbenl]]

When an iframe is navigated and then its page is reloaded, the reloaded iframe is not navigated to its original URL:

async def test_reload_resets_iframe_location(bidi_session, new_tab, inline):
    url = inline("<iframe id='myframe' src='about:blank'></iframe>")
    iframe_url = inline("Hello iframe")

    contexts = await navigate_and_assert(bidi_session, new_tab, url)
    frame = contexts[0]["children"][0]

    await bidi_session.browsing_context.navigate(
        context=frame["context"], url=iframe_url, wait="complete"
    )

    await bidi_session.browsing_context.reload(
        context=new_tab["context"], wait="complete"
    )

    result = await bidi_session.script.evaluate(
        expression="myframe.contentDocument.location.href",
        target=ContextTarget(new_tab["context"]),
        await_promise=False,
    )
    assert result["value"] == "about:blank"

The test passes when checking myframe.src instead of myframe.contentDocument.location.href, so the src attribute of the reloaded iframe is set to the expected URL but its contentDocument isn't.

The issue makes this Playwright test fail (although the test is for a different issue).

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This sounds more like a platform bug. Can you reproduce it manually in Firefox?

Comment 2

[Holger Benl [:hbenl]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #1)

This sounds more like a platform bug. Can you reproduce it manually in Firefox?

No, I couldn't. It would be a pretty serious bug if this was reproducible manually.

Comment 3

[Julian Descottes [:jdescottes]]

DevTools has switched to BrowserCommands.reload:
https://searchfox.org/firefox-main/rev/7b74d86ca4ce65e82fdaa3095d2d39a26d3ac4f6/browser/base/content/browser-commands.js#105-117

reload() {
  if (gBrowser.currentURI.schemeIs("view-source")) {
    // Bug 1167797: For view source, we always skip the cache
    this.reloadSkipCache();
    return;
  }
  gBrowser.reloadWithFlags(Ci.nsIWebNavigation.LOAD_FLAGS_NONE);
},

reloadSkipCache() {
  // Bypass proxy and cache.
  gBrowser.reloadWithFlags(kSkipCacheFlags);
},

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We should most likely fix this in Marionette as well. AFAIK we are using the same method via the browsing context.

Comment 5

[Julian Descottes [:jdescottes]]

Switching to the devtools API doesn't help.
The only thing which does help is to skip the cache for the reload.

(In reply to Holger Benl [:hbenl] from comment #2)

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #1)

This sounds more like a platform bug. Can you reproduce it manually in Firefox?

No, I couldn't. It would be a pretty serious bug if this was reproducible manually.

I can actually reproduce manually and there is a difference between chrome and firefox here.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: iframe1.html

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: iframe_navigation.zip

Can't really easily share a test page with iframes, but if you serve the attached test case, click on the link to navigate to iframe 2 and reload (using the reload button of Firefox/Chrome, not keyboard shortcuts):

• Firefox will still show iframe 2 after the reload
• Chrome will show iframe 1 again (after some delay though ...)

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #5)

Switching to the devtools API doesn't help.
The only thing which does help is to skip the cache for the reload.

Ok interesting, I was trying with tab.linkedBrowser.reloadWithFlags, but this is actually a different codepath. The devtools solution works.

That being said, my test case from the previous comment is still valid, and there is still a difference between Chrome and Firefox. Maybe startingon about:blank makes it slightly different?

Comment 9

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #4)

We should most likely fix this in Marionette as well. AFAIK we are using the same method via the browsing context.

I added a test case for classic, but Marionette doesn't hit the issue because it reloads bypassing the cache by default:

https://searchfox.org/firefox-main/rev/7b74d86ca4ce65e82fdaa3095d2d39a26d3ac4f6/remote/marionette/navigate.sys.mjs#182-185

navigate.refresh = async function (browsingContext) {
  const flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
  browsingContext.reload(flags);
};

Comment 10

[Julian Descottes [:jdescottes]]

The API used by DevTools is not targeting a specific browser/browsingContext, and only works with the selected tab so it can't be used as is.
I tried to bisect the necessary changes, and it seems to boil down to https://searchfox.org/firefox-main/rev/7b74d86ca4ce65e82fdaa3095d2d39a26d3ac4f6/browser/components/tabbrowser/content/tabbrowser.js#786-792

const { browsingContext } = tab.linkedBrowser;
const { sessionHistory } = browsingContext;
if (sessionHistory) {
  sessionHistory.reload(reloadFlags);
} else {
  browsingContext.reload(reloadFlags);
}

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 2023917 - [remote] Use sessionHistory.reload when possible to reset iframe location

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 2023917 - [wdspec] Add tests for iframe location reset after parent frame reload

Comment 13

[Julian Descottes [:jdescottes]]

Phabricator revisions:

• https://phabricator.services.mozilla.com/D289286
• https://phabricator.services.mozilla.com/D289287

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Julian Descottes [:jdescottes] from comment #7)

Created attachment 9556669 [details]
iframe_navigation.zip

Can't really easily share a test page with iframes, but if you serve the attached test case, click on the link to navigate to iframe 2 and reload (using the reload button of Firefox/Chrome, not keyboard shortcuts):

• Firefox will still show iframe 2 after the reload
• Chrome will show iframe 1 again (after some delay though ...)

So it means that there is indeed a bug in Firefox when using the reload button for this scenario? Whether keyboard or the button is used we should produce the same results. If that is the case, we should indeed file a bug for it (if none exists yet).

Comment 15

[Julian Descottes [:jdescottes]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #14)

(In reply to Julian Descottes [:jdescottes] from comment #7)

Created attachment 9556669 [details]
iframe_navigation.zip

Can't really easily share a test page with iframes, but if you serve the attached test case, click on the link to navigate to iframe 2 and reload (using the reload button of Firefox/Chrome, not keyboard shortcuts):

• Firefox will still show iframe 2 after the reload
• Chrome will show iframe 1 again (after some delay though ...)

So it means that there is indeed a bug in Firefox when using the reload button for this scenario? Whether keyboard or the button is used we should produce the same results. If that is the case, we should indeed file a bug for it (if none exists yet).

Yes but it looks like a different scenario compared to the one from Holger for some reason. I can file a bug but I know that reloads triggered via buttons/keyboard shortcuts have historically been different between browsers.

Comment 16

[Julian Descottes [:jdescottes]]

Why did Phabricator Automation override my whiteboard update???

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2a999221c317
https://hg.mozilla.org/integration/autoland/rev/1836450a0e2c
[remote] Use sessionHistory.reload when possible to reset iframe location r=whimboo
https://github.com/mozilla-firefox/firefox/commit/f58a8fea1036
https://hg.mozilla.org/integration/autoland/rev/eed74152771e
[wdspec] Add tests for iframe location reset after parent frame reload r=whimboo

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/58782 for changes under testing/web-platform/tests

Comment 19

[amarc]

https://hg.mozilla.org/mozilla-central/rev/1836450a0e2c
https://hg.mozilla.org/mozilla-central/rev/eed74152771e

Comment 20

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot