Closed
Bug 1267289
Opened 8 years ago
Closed 8 years ago
User typed value is lost in e10s when the load produces an error page without keyword.enabled
Categories
(Firefox :: Address Bar, defect, P1)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 49
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•8 years ago
|
||
(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.
Assignee | ||
Comment 2•8 years ago
|
||
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/
Attachment #8745059 -
Flags: review?(mdeboer)
Attachment #8745059 -
Flags: review?(mconley)
Assignee | ||
Comment 3•8 years ago
|
||
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/
Attachment #8745059 -
Attachment description: MozReview Request: Bug 1267289 - add more URL bar tests and fix issue with error pages, r?mikedeboer,Mossop → MozReview Request: Bug 1267289 - add more URL bar tests and fix issue with error pages, r?mikedeboer,mconley
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
||
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" }`
Attachment #8745059 -
Flags: review?(mdeboer) → review+
Updated•8 years ago
|
tracking-e10s:
--- → ?
Updated•8 years ago
|
Attachment #8745059 -
Flags: review?(mconley) → review+
Comment 6•8 years ago
|
||
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?
Updated•8 years ago
|
Priority: -- → P1
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c3983f53698a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 9•8 years ago
|
||
:Gijs, can we uplift this to 47 and 48?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 10•8 years ago
|
||
(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?
Flags: needinfo?(rkothari)
Flags: needinfo?(jmathies)
Flags: needinfo?(gijskruitbosch+bugs)
Comment 11•8 years ago
|
||
(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.
Flags: needinfo?(jmathies)
(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.
status-firefox47:
--- → wontfix
Flags: needinfo?(rkothari)
Comment 13•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/c560b2cb8b97
You need to log in
before you can comment on or make changes to this bug.
Description
•