Closed
Bug 1241085
Opened 9 years ago
Closed 9 years ago
Private tab clears location bar when finishes loading even if I already typed something (especially noticeable in e10s)
Categories
(Firefox :: Address Bar, defect, P2)
Firefox
Address Bar
Tracking
()
VERIFIED
FIXED
Firefox 49
People
(Reporter: arni2033, Assigned: Gijs)
References
Details
Attachments
(6 files)
(deleted),
video/webm
|
Details | |
(deleted),
text/x-review-board-request
|
mconley
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mconley
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mconley
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
mconley
:
review+
Gijs
:
review+
|
Details |
(deleted),
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
>>> My Info: Win7_64, Nightly 46, 32bit, ID 20160118030338
STR:
1. Select this line of text and copy it to clipboard
2. Open about:preferences#general, enable multiprocess mode, restart the browser
3. Open Private window
4. While private tab is still loading, paste the string copied in Step 1 into urlbar.
That's Ctrl+L, Ctrl+V. You've got 1 second (sometimes even 2 seconds)
5. Wait 5 seconds until private tab is finally loaded
Result: After Step 5 location bar gets cleared, so all input is lost
Expectations: Strings I typed/pasted into urlbar should stay untouched.
Comment 1•9 years ago
|
||
I can reproduce this.
Updated•9 years ago
|
Assignee: nobody → mconley
tracking-e10s:
--- → m9+
Updated•9 years ago
|
Component: Untriaged → Location Bar
Comment 2•9 years ago
|
||
I wonder if Bug 1199934 is the same issue
Looks like the same issue. They both could be resolved if Location bar wasn't allowed to change the value when it's focused, just like I said in bug 814358 comment 5. But I guess there should still be a way for add-ons to do that. Anyway, there was no work on that for years.
Also, I just realized that this bug is reproducible in non-e10s mode, if I perform STR fast enough
It's only happens more often in e10s.
Summary: [e10s] Private tab clears location bar when finishes loading even if I already typed something → Private tab clears location bar when finishes loading even if I already typed something (especially noticeable in e10s)
Updated•9 years ago
|
Comment 4•9 years ago
|
||
(In reply to arni2033 from comment #3)
> Also, I just realized that this bug is reproducible in non-e10s mode, if I
> perform STR fast enough
> It's only happens more often in e10s.
Since the issue appears to be worse in e10s than non-e10s, maybe this should be tracked for e10s.
Comment 5•9 years ago
|
||
I think this is a dupe of bug 798249, but we can keep them separate until bug 798249 is fixed and verify then.
Updated•9 years ago
|
Priority: -- → P2
Comment 6•9 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> I think this is a dupe of bug 798249, but we can keep them separate until
> bug 798249 is fixed and verify then.
This was not fixed by bug 798249. I can still reproduce on the Nightly from 2016-03-28.
Updated•9 years ago
|
Assignee | ||
Comment 7•9 years ago
|
||
Mike, can I steal this or are you actively working on it?
Flags: needinfo?(mconley)
Comment 8•9 years ago
|
||
You can absolutely take this.
Assignee: mconley → gijskruitbosch+bugs
Flags: needinfo?(mconley)
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 9•9 years ago
|
||
There are 3 cases which are somewhat interesting here:
1) open new tab. Load invalid URI. You want that URI to stay visible, and the pageproxystate of the URL bar should be "invalid" (no lock icon, no way to get the identity dropdown to show up at all, it should be greyed out)
2) open new tab. Load valid HTTPS URI. You want that URI to stay visible, potentially change with redirects, and you want the pageproxystate of the URL bar to be "valid" (so you get a lock icon, can inspect the security state, etc.)
3) open new private browsing window. This implicitly loads about:privatebrowsing into the initially empty (about:blank) tab. You would expect this to not change the URL bar value, because it's the initial load, and in a manner of speaking, it was not triggered by you - certainly not by URL bar input.
Unfortunately, because of the e10s process type switch, all these 3 cases hit the same code path, here:
https://hg.mozilla.org/integration/fx-team/rev/c3983f53698a#l5.16
In bug 1267289 I fixed case (1).
The unfortunate thing is that the current code is very hard to grok - lots of places increment/decrement userTypedClear, sometimes by more than 1, and it's a bit of a mess. I have a patch locally that effectively just sets a bool instead of this increment of userTypedClear.
The current state is then that (1) and (2) are working but (3) is broken. If I get rid of that boolean setting, (1) and (3) are working, and (2) is broken.
For the code here, there is no discernible difference between these 3 cases.
I eventually ended up reworking the code so sessionstore generally does not need to worry about setting userTypedValue except when actually restoring something, and updated code in tabbrowser.xml so that the load of about:privatebrowsing/about:newtab/about:home no longer get treated as user-initiated loads for newly-opened browsers. Patches hopefully shortly.
Assignee | ||
Comment 10•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49537/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49537/
Attachment #8746685 -
Flags: review?(mconley)
Attachment #8746686 -
Flags: review?(mconley)
Attachment #8746687 -
Flags: review?(mconley)
Assignee | ||
Comment 11•9 years ago
|
||
userTypedClear was used for two cases:
1) to keep track of whether we were in the middle of a loadURI call. This use is replaced by inLoadURI, which is
more sane when using e10s (though it's hard to be precise there because we're sending all web navigation calls to
the content process and this introduces a degree of asynchronousness that we just have to live with...).
2) to keep track of whether we were between a network start and a corresponding network stop, and whether the user
typed since the load properly started. This is now tracked on a small object on the browser binding, which has
appropriately named method so we're not just incrementing some magic number but actually understand what
we're saying, and so the information we get out (did the user type since this load started or not?) makes sense.
Note that we're keeping userTypedClear in session store information in order to remain backwards compatible.
It becomes a simple boolean-stored-as-int (1 or 0) that indicates whether we quit/crashed/stopped while a load
was pending, or not.
Review commit: https://reviewboard.mozilla.org/r/49539/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49539/
Assignee | ||
Comment 12•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/49541/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49541/
Updated•9 years ago
|
Attachment #8746685 -
Flags: review?(mconley) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8746685 [details]
MozReview Request: Bug 1241085 - part 1: improve inLoadURI support, r=mconley
https://reviewboard.mozilla.org/r/49537/#review46487
::: toolkit/content/browser-child.js:291
(Diff revision 1)
> + if (this.webNavigation.canGoBack)
> + this._wrapURIChangeCall(() => this.webNavigation.goBack());
Aren't we supposed to be wrapping one-liners in braces now? Not a big deal, just curious.
*can't wait for mozreview bots*
::: toolkit/content/browser-child.js:304
(Diff revision 1)
>
> gotoIndex: function(index) {
> - this.webNavigation.gotoIndex(index);
> + this._wrapURIChangeCall(() => this.webNavigation.gotoIndex(index));
> },
>
> loadURI: function(uri, flags, referrer, referrerPolicy, postData, headers, baseURI) {
I guess we don't need to do the wrapping the URI change stuff in loadURI? Can you quickly explain why?
Updated•9 years ago
|
Attachment #8746686 -
Flags: review?(mconley) → review+
Comment 14•9 years ago
|
||
Comment on attachment 8746686 [details]
MozReview Request: Bug 1241085 - part 2: rip out userTypedClear and replace it with more self-documenting stuff, r=mconley
https://reviewboard.mozilla.org/r/49539/#review46491
::: browser/base/content/tabbrowser.xml:621
(Diff revision 1)
>
> var oldBlank = this.mBlank;
>
> const nsIWebProgressListener = Components.interfaces.nsIWebProgressListener;
> const nsIChannel = Components.interfaces.nsIChannel;
> + var location, originalLocation;
let, not var
::: browser/base/content/tabbrowser.xml
(Diff revision 1)
> - <property name="userTypedClear"
> - onget="return this.mCurrentBrowser.userTypedClear;"
> - onset="return this.mCurrentBrowser.userTypedClear = val;"/>
Good riddance.
::: browser/components/sessionstore/test/browser_522545.js:31
(Diff revision 1)
> - is(browser.userTypedClear, 0,
> - "userTypeClear restored as expected");
> + is(browser.didStartLoadSinceLastUserTyping(), false,
> + "We still know that no load is ongoing");
Alternatively,
```JavaScript
ok(!browser.didStartLoadSinceLastUserTyping(), "We still know that no load is ongoing");
```
But meh.
::: toolkit/content/widgets/browser.xml:773
(Diff revision 1)
> - the location bar to be updated to the new URL.
> -
> + startedLoad(url) {
> + this._startedLoadSinceLastUserTyping = true;
The url is never used here.
Comment 15•9 years ago
|
||
Comment on attachment 8746687 [details]
MozReview Request: Bug 1241085 - part 3: actually fix about:privatebrowsing clearing the URL bar, r=mconley
https://reviewboard.mozilla.org/r/49541/#review46495
Thanks for the test!
Attachment #8746687 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 16•9 years ago
|
||
This hasn't landed yet because the sessionstore test I'm modifying is breaking with these changes, and I have not yet succeeded in reproducing any of the varied breakage listed locally. :-\
Assignee | ||
Comment 17•9 years ago
|
||
https://reviewboard.mozilla.org/r/49537/#review47467
::: toolkit/content/browser-child.js:291
(Diff revision 1)
> + if (this.webNavigation.canGoBack)
> + this._wrapURIChangeCall(() => this.webNavigation.goBack());
Not sure, but updating this anyway.
::: toolkit/content/browser-child.js:304
(Diff revision 1)
>
> gotoIndex: function(index) {
> - this.webNavigation.gotoIndex(index);
> + this._wrapURIChangeCall(() => this.webNavigation.gotoIndex(index));
> },
>
> loadURI: function(uri, flags, referrer, referrerPolicy, postData, headers, baseURI) {
We're already setting `_inLoadURI` 'manually' in this method. I'll update it to use the wrapper function, though.
Comment 18•9 years ago
|
||
Assignee | ||
Comment 19•9 years ago
|
||
Much to my chagrin I felt obliged to back this out when I realized that if you:
1) start firefox, clean profile
2) open about:preferences
3) turn on session restore ('when firefox starts up, show my tabs from last time')
4) open a new tab
5) close firefox
6) reopen firefox
now about:newtab is stuck in your url bar. Haven't investigated why yet, but this seemed like enough of an issue to back this out over.
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/de5ab3fd7c7e
Comment 20•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c86861912bb
https://hg.mozilla.org/mozilla-central/rev/386b9c750bd2
https://hg.mozilla.org/mozilla-central/rev/b386e97721cf
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 21•9 years ago
|
||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 22•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51033/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51033/
Attachment #8746685 -
Attachment description: MozReview Request: Bug 1241085 - part 1: improve inLoadURI support, r?mconley → MozReview Request: Bug 1241085 - part 1: improve inLoadURI support, r=mconley
Attachment #8746686 -
Attachment description: MozReview Request: Bug 1241085 - part 2: rip out userTypedClear and replace it with more self-documenting stuff, r?mconley → MozReview Request: Bug 1241085 - part 2: rip out userTypedClear and replace it with more self-documenting stuff, r=mconley
Attachment #8746687 -
Attachment description: MozReview Request: Bug 1241085 - part 3: actually fix about:privatebrowsing clearing the URL bar, r?mconley → MozReview Request: Bug 1241085 - part 3: actually fix about:privatebrowsing clearing the URL bar, r=mconley
Attachment #8749566 -
Flags: review?(mconley)
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8746685 [details]
MozReview Request: Bug 1241085 - part 1: improve inLoadURI support, r=mconley
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49537/diff/1-2/
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8746686 [details]
MozReview Request: Bug 1241085 - part 2: rip out userTypedClear and replace it with more self-documenting stuff, r=mconley
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49539/diff/1-2/
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8746687 [details]
MozReview Request: Bug 1241085 - part 3: actually fix about:privatebrowsing clearing the URL bar, r=mconley
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49541/diff/1-2/
Assignee | ||
Comment 26•9 years ago
|
||
https://reviewboard.mozilla.org/r/51033/#review47705
::: browser/components/sessionstore/test/browser_newtab_userTypedValue.js:6
(Diff revision 1)
> + let tabOpenedAndSwitchedTo = BrowserTestUtils.switchTab(win.gBrowser, () => {});
> + win.BrowserOpenTab();
> + let tab = yield tabOpenedAndSwitchedTo;
FWIW, I tried rolling about:newtab into the loop further down, but it doesn't work well with openNewForegroundTab (which implicitly waits for the tab to load), so I couldn't do that... :-\
(I initially just tested the about:newtab case and then expanded the test because I figured other initial pages should also be tested.)
Comment 27•9 years ago
|
||
backout bugherder |
backout from m-c
https://hg.mozilla.org/mozilla-central/rev/de5ab3fd7c7e backout from m-c
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 29•9 years ago
|
||
Comment on attachment 8749566 [details]
MozReview Request: Bug 1241085 - fix issues with about:newtab and other initial pages whose URIs now persist after session restore, r?mconley
https://reviewboard.mozilla.org/r/51033/#review47851
::: browser/components/sessionstore/test/browser_newtab_userTypedValue.js:3
(Diff revision 1)
> +"use strict";
> +
> +add_task(function* () {
Mind adding a comment about what's being tested up here?
::: browser/components/sessionstore/test/browser_newtab_userTypedValue.js:22
(Diff revision 1)
> + ok(SessionStore.getClosedWindowCount(), "Should have a closed window");
> +
> + win = SessionStore.undoCloseWindow(0);
> + yield TestUtils.topicObserved("sessionstore-single-window-restored",
> + subject => subject == win);
> + yield BrowserTestUtils.waitForMessage(win.gBrowser.selectedBrowser.messageManager, "SessionStore:update");
I think it's more traditional to do a message flush like:
```JavaScript
yield TabStateFlusher.flush(win.gBrowser.selectedBrowser);
```
::: browser/components/sessionstore/test/browser_newtab_userTypedValue.js:33
(Diff revision 1)
> + if (url == BROWSER_NEW_TAB_URL) {
> + continue; // We tested this above.
> + }
Nope, we didn't. Probably should mention the "didn't work so well" details in that comment instead.
Might be worth filing a follow-up to make this test work with about:newtab too, since that's likely the most common case.
::: browser/components/sessionstore/test/browser_newtab_userTypedValue.js:44
(Diff revision 1)
> + ok(SessionStore.getClosedWindowCount(), "Should have a closed window");
> +
> + win = SessionStore.undoCloseWindow(0);
> + yield TestUtils.topicObserved("sessionstore-single-window-restored",
> + subject => subject == win);
> + yield BrowserTestUtils.waitForMessage(win.gBrowser.selectedBrowser.messageManager, "SessionStore:update");
As above, with `TabStateFlusher.flush`
Attachment #8749566 -
Flags: review?(mconley) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8749566 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Comment on attachment 8749566 [details]
MozReview Request: Bug 1241085 - fix issues with about:newtab and other initial pages whose URIs now persist after session restore, r?mconley
https://reviewboard.mozilla.org/r/51033/#review48115
::: browser/components/sessionstore/test/browser_newtab_userTypedValue.js:22
(Diff revision 1)
> + ok(SessionStore.getClosedWindowCount(), "Should have a closed window");
> +
> + win = SessionStore.undoCloseWindow(0);
> + yield TestUtils.topicObserved("sessionstore-single-window-restored",
> + subject => subject == win);
> + yield BrowserTestUtils.waitForMessage(win.gBrowser.selectedBrowser.messageManager, "SessionStore:update");
Note that I also need to wait for the tab to load if using TabStateFlusher (though I can't do that in the about:newtab case because the tab is preloaded).
I hope this won't lead to intermittents...
::: browser/components/sessionstore/test/browser_newtab_userTypedValue.js:33
(Diff revision 1)
> + if (url == BROWSER_NEW_TAB_URL) {
> + continue; // We tested this above.
> + }
We did, actually: that's the use of BrowserOpenTab() on line 7 of this revision, which opens about:newtab.
Assignee | ||
Comment 31•9 years ago
|
||
Assignee | ||
Comment 32•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #31)
> remote:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7096e91fb2e2
Grumble. Still orange with a timeout and no useful information. Let's see if it's really just slow:
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b822982220e6
Assignee | ||
Comment 33•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #32)
> (In reply to :Gijs Kruitbosch from comment #31)
> > remote:
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7096e91fb2e2
>
> Grumble. Still orange with a timeout and no useful information. Let's see if
> it's really just slow:
>
> remote:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b822982220e6
*tableflip*
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f0c49991edd
(I tried reproducing on a local vm, nada.)
Assignee | ||
Comment 34•9 years ago
|
||
So it turns out requestLongerTimeout only works if you pass it something > 1, and the problem really was that we're finishing at the 62 second mark on infra, with the default timeout being 1 minute, which obviously doesn't work. That's not really self-documenting. :-\
Here was my successful trypush:
remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d8509b69a70
and now I landed again:
remote: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=e4260b4769a0
Comment 35•9 years ago
|
||
Comment 36•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70478bee1641
https://hg.mozilla.org/mozilla-central/rev/048946d0ad2d
https://hg.mozilla.org/mozilla-central/rev/bfd66f561a67
https://hg.mozilla.org/mozilla-central/rev/e4260b4769a0
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 37•9 years ago
|
||
Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: various issues with the location bar losing user-typed input, especially under e10s
[Describe test coverage new/current, TreeHerder]: comes with a bunch of tests!
[Risks and why]: medium to low. This is a lot of code, but it's all baked on Nightly for 1-3* weeks now, comes with a lot of tests to verify its behaviour, and I explicitly looked for URL-related regression bugs filed over the past few weeks and did not see any. I think getting this into aurora in time for a week of extra baking followed by a full beta cycle is OK. It will improve stability and functionality especially in e10s, and seeing as there's a non-zero chance we'll release that with 48, having this in the tree for 48 seems prudent.
[String/UUID change made/needed]: no.
* I've folded several patches to make it easier to uplift the entire dep tree here
(this patch has bug 1267289, bug 1272317 and bug 1270067 folded in)
Attachment #8756883 -
Flags: review+
Attachment #8756883 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Comment 38•9 years ago
|
||
Comment on attachment 8756883 [details] [diff] [review]
Patch for Aurora
OK, let's try that to polish e10s.
Please update the status of the 3 other bugs to fixed when this patch landed.
Thanks
Attachment #8756883 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 39•9 years ago
|
||
bugherder uplift |
Comment 40•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #38)
> Comment on attachment 8756883 [details] [diff] [review]
> Patch for Aurora
>
> OK, let's try that to polish e10s.
> Please update the status of the 3 other bugs to fixed when this patch landed.
> Thanks
made sure that this marking happened via our bugherder automation
Comment 42•8 years ago
|
||
I have reproduced this Bug on Nightly 46.0a1 (2016-01-20) on Windows 10, 64 Bit!
The bug's fix is now verified on latest Aurora 49.0a2 and Nightly 49.0a1.
Aurora 49.0a2:
Build ID 20160723004004
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Nightly 49.0a1:
Build ID 20160602030220
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0
Assignee | ||
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•