1978075 - sync-about-blank: "emulation.setLocaleOverride" doesn't persist past reloads

Status: RESOLVED DUPLICATE of bug 1978533

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

Description

[Vincent Hilla [:vhilla]]

I just rebased bug 543435 (sync-about-blank) onto bug 1968952 (add "emulation.setLocaleOverride") and /webdriver/tests/bidi/emulation/set_locale_override/user_contexts.py subtest test_set_to_user_context_and_then_to_context is failing. There might be two aspects.

In bug 1967705, I added a blocking mechanism to browsing_context.create so that it waits for the configuration module to complete, given about:blank cannot be parser-blocked. Maybe we should do the same for browsing_context.reload.

I wrote a prototype for blocking reload on configuration, but that didn't help. I checked from in the logs that during the reload, context.languageOverride is set and BrowsingContext::DidSet(FieldIndex<IDX_LanguageOverride>, nsString&& aOldValue) called. But it early-returns as the value didn't change while JS_GetDefaultLocale doesn't anymore correspond to the override.

Comment 1

[Vincent Hilla [:vhilla]]

Interestingly, I tried adding a test to dom/base/test/browser_language_override.js and that seemed to work. The override does stay through something like this:

tab.linkedBrowser.reloadWithFlags(Ci.nsIWebNavigation.LOAD_FLAGS_NONE);
await BrowserTestUtils.browserLoaded(browser);
await assertLanguageOverridden(browser, languageOverride);

Comment 2

[Vincent Hilla [:vhilla]]

pernosco for user_context.py: https://pernos.co/debug/EBgIDtXuAm_1fvZHnAPmqg/index.html

I'm confused. Before and after reload, the call to JSRuntime::getDefaultLocale has the same context (at least by memory location) and takes the early return for defaultLocale. There's no call to JSRuntime::setDefaultLocale in between, but the return value changes. I changed defaultLocale to private, no compile errors, same issue. Though the process seems to change.

Comment 3

[Vincent Hilla [:vhilla]]

Ah, reloading seems to preserve the JS runtime, but a cross-process redirect doesn't. So I found two issues.

---

Bug 1968951 doesn't re-apply the override if the JS runtime changes. In this case, this happens as a cross-process redirect changes the content process. I attached a variation of browser_language_override.js to test this. Note that BrowsingContext languageOverride is unchanged and it doesn't work to just set it to the override again.

const CROSS_PAGE_URL = PAGE_URL.replace(".com", ".org");
await BrowserTestUtils.startLoadingURIString(browser, CROSS_PAGE_URL);
await BrowserTestUtils.browserLoaded(browser, false, CROSS_PAGE_URL);
is(browsingContext.languageOverride, languageOverride);  // pass
await assertLanguageOverridden(browser, languageOverride);  // fail

---

For some reason, reload in user_context.py causes a cross-process redirect with bug 543435...

Comment 4

[Vincent Hilla [:vhilla]]

Attachment: browser_language_override.js

Comment 5

[Vincent Hilla [:vhilla]]

I worry the override also won't be applied to new subframes.

Comment 6

[Alexandra Borovova [:Sasha]]

(In reply to Vincent Hilla [:vhilla] from comment #5)

I worry the override also won't be applied to new subframes.

Yes, looks like that's right. I've created a separate bug 1978533 for it, and I will fix it now. In the scope of the fix I'm going to remove the restriction for only setting the value when it's different from the one already set on browsing context. I think that should probably help with reload as well? We maybe still have to add a waiting mechanics for reload on about:blank, but then it should fix the test_set_to_user_context_and_then_to_context test, right?

Comment 7

[Vincent Hilla [:vhilla]]

Attachment: WIP: Bug 1978075 - browsing_context.reload wait for configuration complete

Comment 8

[Vincent Hilla [:vhilla]]

Yes, always setting the value on the runtime and having a waiting mechanic for browsing_context.reload should fix test_set_to_user_context_and_then_to_context. I attached my WIP patch for such a mechanic.

I think a cross-process navigation outside of webdriver might continue to evade the locale override. See the attached browser test. But maybe we don't care about that.

Thanks for taking bug 1978533!

Comment 9

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1978075 - Allow setting languageOverride even if the browsing context already holds the same value.

Comment 10

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1978075 - [bidi] Apply the locale override also when iframes are created.

Comment 11

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1978075 - [wdspec] Add test for calling "emulation.setLocaleOverride" on the page with iframes.

Comment 12

[Phabricator Automation]

Comment on attachment 9502086 [details]
Bug 1978075 - Allow setting languageOverride even if the browsing context already holds the same value.

Revision D258193 was moved to bug 1978533. Setting attachment 9502086 [details] to obsolete.

Comment 13

[Phabricator Automation]

Comment on attachment 9502087 [details]
Bug 1978075 - [bidi] Apply the locale override also when iframes are created.

Revision D258194 was moved to bug 1978533. Setting attachment 9502087 [details] to obsolete.

Comment 14

[Phabricator Automation]

Comment on attachment 9502088 [details]
Bug 1978075 - [wdspec] Add test for calling "emulation.setLocaleOverride" on the page with iframes.

Revision D258195 was moved to bug 1978533. Setting attachment 9502088 [details] to obsolete.

Comment 15

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

Vincent, I assume this blocks bug 543435 instead?

Comment 16

[Vincent Hilla [:vhilla]]

I haven't seen this test fail anymore since bug 1978533 landed. But I suppose an explicit blocking mechanism would be good anyway.

Comment 17

[Alexandra Borovova [:Sasha]]

I think after bug 1980211 lands, we actually shouldn't need the blocking mechanism for reload anymore.

Comment 18

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

(In reply to Alexandra Borovova [:Sasha] from comment #17)

I think after bug 1980211 lands, we actually shouldn't need the blocking mechanism for reload anymore.

Vincent, can you please re-check? Thanks!

Comment 19

[Vincent Hilla [:vhilla]]

I agree, a blocking mechanism for reload isn't needed right now.

---

I'll briefly document my reasoning.

I ensured the existing tests and my attached tests all pass (with --verify). I'm not familiar enough with the JS code, but I think it's reasonable to assume all overrides applied by _ConfigurationModule would survive reloads on their own. I also tested this for preload scripts and will add a test case to add_preload_script.py.

Moreover, a reload should not cause this initial about:blank handling, as it's not the first load of the docshell anymore. So it should be a normal load that can be blocked.