1241085 - Private tab clears location bar when finishes loading even if I already typed something (especially noticeable in e10s)

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[arni2033]

Attachment: screencast 1 - [e10s] Private tab clears location bar.webm

>>>   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

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

I can reproduce this.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

I wonder if Bug 1199934 is the same issue

Comment 3

[arni2033]

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.

Comment 4

[Tanvi Vyas[:tanvi]]

(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

[Jared Wein [:jaws] (please needinfo? me)]

I think this is a dupe of bug 798249, but we can keep them separate until bug 798249 is fixed and verify then.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 7

[:Gijs (he/him)]

Mike, can I steal this or are you actively working on it?

Comment 8

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

You can absolutely take this.

Comment 9

[:Gijs (he/him)]

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.

Comment 10

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1241085 - part 1: improve inLoadURI support, r=mconley

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

Comment 11

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1241085 - part 2: rip out userTypedClear and replace it with more self-documenting stuff, r=mconley

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/

Comment 12

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1241085 - part 3: actually fix about:privatebrowsing clearing the URL bar, r=mconley

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

Comment 13

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

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?

Comment 14

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

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

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

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!

Comment 16

[:Gijs (he/him)]

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. :-\

Comment 17

[:Gijs (he/him)]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c86861912bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/386b9c750bd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b386e97721cf

Comment 19

[:Gijs (he/him)]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c86861912bb
https://hg.mozilla.org/mozilla-central/rev/386b9c750bd2
https://hg.mozilla.org/mozilla-central/rev/b386e97721cf

Comment 21

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/e5a10bc7dac4

Comment 22

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1241085 - fix issues with about:newtab and other initial pages whose URIs now persist after session restore, r?mconley

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

Comment 23

[:Gijs (he/him)]

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/

Comment 24

[:Gijs (he/him)]

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/

Comment 25

[:Gijs (he/him)]

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/

Comment 26

[:Gijs (he/him)]

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

[Carsten Book [:Tomcat]]

backout from m-c
https://hg.mozilla.org/mozilla-central/rev/de5ab3fd7c7e backout from m-c

Comment 28

[:Gijs (he/him)]

:-\

Comment 29

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

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`

Comment 30

[:Gijs (he/him)]

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.

Comment 31

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=7096e91fb2e2

Comment 32

[:Gijs (he/him)]

(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

Comment 33

[:Gijs (he/him)]

(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.)

Comment 34

[:Gijs (he/him)]

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

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/70478bee1641
https://hg.mozilla.org/integration/fx-team/rev/048946d0ad2d
https://hg.mozilla.org/integration/fx-team/rev/bfd66f561a67
https://hg.mozilla.org/integration/fx-team/rev/e4260b4769a0

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 37

[:Gijs (he/him)]

Attachment: Patch for Aurora

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)

Comment 38

[Sylvestre Ledru [:Sylvestre]]

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

Comment 39

[Carsten Book [:Tomcat]]

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

Comment 40

[Carsten Book [:Tomcat]]

(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

[Saddam Hossain]

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