1267289 - User typed value is lost in e10s when the load produces an error page without keyword.enabled

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[:Gijs (he/him)]

STR:

1. in about:config, toggle keyword.enabled to false (this disables search from the URL bar)

2. open a new tab
3. in the new tab, type "foo bar" (note space in the middle) and hit enter

ER:
An error page loads, and the URL bar continues to contain "foo bar";

AR:
An error page loads, and the URL bar is blanked;

This is broken in e10s, but not in non-e10s. It seems to be because we're switching process type AND there are no history entries for the tab. I will add a patch and a number of tests hopefully shortly.

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #0)
> This is broken in e10s, but not in non-e10s. It seems to be because we're
> switching process type AND there are no history entries for the tab. I will
> add a patch and a number of tests hopefully shortly.

Hah, and then I found out I'm breaking something else that my new tests weren't catching, so this is going to need to wait until I've figure out how not to break it.

Comment 2

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1267289 - add more URL bar tests and fix issue with error pages, r?mikedeboer,mconley

This adds tests for issues brought up in bug 231393, bug 264610, bug 302575 and bug 1129564,
all of which fed into the current implementation of userTypedClear/userTypedValue. I intend
to move us away from userTypedClear, but I'm keen not to regress any of these issues, so
I'm adding automated tests to ensure that doesn't happen.

Review commit: https://reviewboard.mozilla.org/r/48799/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48799/

Comment 3

[:Gijs (he/him)]

Comment on attachment 8745059 [details]
MozReview Request: Bug 1267289 - add more URL bar tests and fix issue with error pages, r?mikedeboer,mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48799/diff/1-2/

Comment 4

[:Gijs (he/him)]

I was planning to ask Dave for feedback here, but he's not accepting review requests, so Mikes, I've picked on you instead... let me know if I need to find other "victims"...

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8745059 [details]
MozReview Request: Bug 1267289 - add more URL bar tests and fix issue with error pages, r?mikedeboer,mconley

https://reviewboard.mozilla.org/r/48799/#review45799

r=me with comments addressed on all the testing bits. Well done on those, btw!
The SessionStore bits look fine to me as well, because, hey, otherwise the tests wouldn't work!

::: browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js:1
(Diff revision 2)
> +"use strict";

Nit: can you add a CC-license block here?

::: browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js:5
(Diff revision 2)
> +"use strict";
> +
> +/**
> + * Check that navigating through both the URL bar and using in-page hash/ref-
> + * based links and back/forward navigation updates the URL bar and identity block correctly.

nit: 'hash/ ref-' and 'back/ forward'

::: browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js:7
(Diff revision 2)
> +
> +/**
> + * Check that navigating through both the URL bar and using in-page hash/ref-
> + * based links and back/forward navigation updates the URL bar and identity block correctly.
> + */
> +add_task(function*() {

General nit: I think generally we use `function* () {` (added space between `*` and `(`), but I don't see that used consistently across our modules. But since that's the case with almost every code convention we use, I'm mentioning it anyway!

::: browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js:10
(Diff revision 2)
> + * based links and back/forward navigation updates the URL bar and identity block correctly.
> + */
> +add_task(function*() {
> +  let baseURL = "https://example.org/browser/browser/base/content/test/urlbar/dummy_page.html";
> +  let url = baseURL + "#foo";
> +  yield BrowserTestUtils.withNewTab({gBrowser, url}, function(browser) {

nit: `{ gBrowser, url }`

::: browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js:43
(Diff revision 2)
> +    gURLBar.select();
> +    EventUtils.sendKey("return");
> +
> +    yield locationChangePromise;
> +    verifyURLBarState("after hitting enter on the same URL a second time");
> +

Nit: superfluous newline.

::: browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js:54
(Diff revision 2)
> +
> +    yield locationChangePromise;
> +    verifyURLBarState("after a URL bar hash navigation");
> +
> +    expectURL(baseURL + "#foo");
> +    yield ContentTask.spawn(browser, "", function() {

nit: s/""/null/

::: browser/base/content/test/urlbar/browser_urlbarHashChangeProxyState.js:104
(Diff revision 2)
> +  yield BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> +
> +  is(gURLBar.textValue, url, "URL bar visible value should be correct when the page loads from about:newtab");
> +  is(gURLBar.value, url, "URL bar value should be correct when the page loads from about:newtab");
> +  let identityBox = document.getElementById("identity-box");
> +  ok(identityBox.classList.contains("verifiedDomain"), "Identity box should know we're doing SSL when the page loads from about:newtab");

Nit: my gut tells me this exceeds 120chars, but I could be wrong. Do you have vertical rulers set in your editor config?

::: browser/base/content/test/urlbar/browser_urlbarKeepStateAcrossTabSwitches.js:1
(Diff revision 2)
> +"use strict";

Nit: missing license.

::: browser/base/content/test/urlbar/browser_urlbarKeepStateAcrossTabSwitches.js:24
(Diff revision 2)
> +  is(gURLBar.textValue, input, "Text is still in URL bar after tab switch");
> +  yield BrowserTestUtils.removeTab(tab);
> +});
> +
> +/**
> + * Different type of failure:

Can you finish this comment?

::: browser/base/content/test/urlbar/browser_urlbarKeepStateAcrossTabSwitches.js:27
(Diff revision 2)
> +
> +/**
> + * Different type of failure:
> + */
> +add_task(function*() {
> +  let input = "blah blah blah blah blah";

nit: I vote for a more input with a more creative vibe! ;-)

::: browser/base/content/test/urlbar/browser_urlbarKeepStateAcrossTabSwitches.js:28
(Diff revision 2)
> +/**
> + * Different type of failure:
> + */
> +add_task(function*() {
> +  let input = "blah blah blah blah blah";
> +  yield new Promise(r => SpecialPowers.pushPrefEnv({set: [["keyword.enabled", false]]}, r));

Nit: I don't quite see what shortening 'resolve' to 'r' gives us, but I'm finding it not helping legibility... IMHO.

::: browser/base/content/test/urlbar/browser_urlbarUpdateForDomainCompletion.js:1
(Diff revision 2)
> +"use strict";

Nit: missing license.

::: browser/base/content/test/urlbar/browser_urlbarUpdateForDomainCompletion.js:9
(Diff revision 2)
> + * Disable keyword.enabled (so no keyword search), and check that when you type in
> + * "example" and hit enter, the browser loads and the URL bar is updated accordingly.
> + */
> +add_task(function*() {
> +  yield new Promise(resolve => SpecialPowers.pushPrefEnv({set: [["keyword.enabled", false]]}, resolve));
> +  yield BrowserTestUtils.withNewTab({gBrowser, url: "about:blank"}, function*(browser) {

nit: `{ gBrowser, url: "about:blank" }`

Comment 6

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

Comment on attachment 8745059 [details]
MozReview Request: Bug 1267289 - add more URL bar tests and fix issue with error pages, r?mikedeboer,mconley

https://reviewboard.mozilla.org/r/48799/#review45835

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:594
(Diff revision 2)
> +      browser.addEventListener("XULFrameLoaderCreated", function onXFLC() {
> +        browser.removeEventListener("XULFrameLoaderCreated", onXFLC);
> +        browser.messageManager.addMessageListener("Browser:Init", function onInit() {
> +          browser.messageManager.removeMessageListener("Browser:Init", onInit);
> +          waitForLoad().then(resolve, reject);
> +        });

Can we not listen for the TabRemotenessChange event instead here?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c3983f53698a

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3983f53698a

Comment 9

[Jim Mathies [:jimm]]

:Gijs, can we uplift this to 47 and 48?

Comment 10

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from comment #9)
> :Gijs, can we uplift this to 47 and 48?

I wouldn't be comfortable uplifting this to 47. AIUI we're not shipping e10s to general release in 47 so I don't think this needs to be fixed for 47. Am I misunderstanding? The patch also links into other things (browser.inLoadURI, notably) that aren't in 47, so I don't know if it'd "Just Work" there.

48 is more palatable. The userTypedClear hack stuff is evil and I killed it in bug 1241085. Uplifting that together with this one would make me happier, but that's too big a change for 47 and I'd want to wait on uplifting 1241085 to aurora until some more verification has happened and we've shaken out any potential regressions on Nightly. Ritu/Jim, can you clarify if that idea works and/or if I'm missing anything here?

Comment 11

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Jim Mathies [:jimm] from comment #9)
> > :Gijs, can we uplift this to 47 and 48?
> 
> I wouldn't be comfortable uplifting this to 47. AIUI we're not shipping e10s
> to general release in 47 so I don't think this needs to be fixed for 47. Am
> I misunderstanding? The patch also links into other things
> (browser.inLoadURI, notably) that aren't in 47, so I don't know if it'd
> "Just Work" there.
> 
> 48 is more palatable. The userTypedClear hack stuff is evil and I killed it
> in bug 1241085. Uplifting that together with this one would make me happier,
> but that's too big a change for 47 and I'd want to wait on uplifting 1241085
> to aurora until some more verification has happened and we've shaken out any
> potential regressions on Nightly. Ritu/Jim, can you clarify if that idea
> works and/or if I'm missing anything here?

We have a shot at getting e10s out in 47. If you don't feel comfortable uplifting to beta don't, any initial rollout would be to a limited set of users.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to :Gijs Kruitbosch from comment #10)
> (In reply to Jim Mathies [:jimm] from comment #9)
> > :Gijs, can we uplift this to 47 and 48?
> 
> I wouldn't be comfortable uplifting this to 47. AIUI we're not shipping e10s
> to general release in 47 so I don't think this needs to be fixed for 47. Am
> I misunderstanding? The patch also links into other things
> (browser.inLoadURI, notably) that aren't in 47, so I don't know if it'd
> "Just Work" there.
> 
> 48 is more palatable. The userTypedClear hack stuff is evil and I killed it
> in bug 1241085. Uplifting that together with this one would make me happier,
> but that's too big a change for 47 and I'd want to wait on uplifting 1241085
> to aurora until some more verification has happened and we've shaken out any
> potential regressions on Nightly. Ritu/Jim, can you clarify if that idea
> works and/or if I'm missing anything here?

Thanks Gijs and Jimm. I agree with your recommendation Gijs. 

The current plan of record is that e10s is off by default for Fx47. Up until now (3 weeks of Beta47 cycle), I have taken several uplifts that are e10s related only if they were critical end-user scenarios and/or e10s telemetry/OOMs/stability related. As such this bug does not seem so critical (given the pref flip in STR), let's wontfix for Fx47.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c560b2cb8b97