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

VERIFIED FIXED in Firefox 48

Status

()

defect
P2
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: arni2033, Assigned: Gijs)

Tracking

Trunk
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox46 affected, firefox47 affected, firefox48 verified, firefox49 verified)

Details

Attachments

(6 attachments)

Reporter

Description

4 years ago
>>>   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.
I can reproduce this.
Assignee: nobody → mconley
tracking-e10s: --- → m9+

Updated

3 years ago
Component: Untriaged → Location Bar
I wonder if Bug 1199934 is the same issue
Reporter

Comment 3

3 years ago
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)
(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.
Blocks: 1247816
I think this is a dupe of bug 798249, but we can keep them separate until bug 798249 is fixed and verify then.
Reporter

Updated

3 years ago
See Also: → 798249
(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.
Assignee

Comment 7

3 years ago
Mike, can I steal this or are you actively working on it?
Flags: needinfo?(mconley)
You can absolutely take this.
Assignee: mconley → gijskruitbosch+bugs
Flags: needinfo?(mconley)
Assignee

Updated

3 years ago
See Also: → 891452
Assignee

Updated

3 years ago
Depends on: 1266475
Assignee

Updated

3 years ago
Depends on: 1267289
Assignee

Comment 9

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

3 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/
Attachment #8746685 - Flags: review?(mconley) → review+
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?
Attachment #8746686 - Flags: review?(mconley) → review+
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 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

3 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

Updated

3 years ago
Depends on: 1270067
Assignee

Comment 17

3 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.
Assignee

Comment 19

3 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 22

3 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

3 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

3 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

3 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

3 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

3 years ago
backoutbugherder
backout from m-c
https://hg.mozilla.org/mozilla-central/rev/de5ab3fd7c7e backout from m-c
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Assignee

Comment 28

3 years ago
:-\
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

3 years ago
Attachment #8749566 - Flags: review+
Assignee

Comment 30

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

3 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

3 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

3 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

Updated

3 years ago
Blocks: 1272401
Assignee

Comment 37

3 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?
Reporter

Updated

3 years ago
Depends on: 1276117
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+
(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
Assignee

Updated

3 years ago
Blocks: 1277060

Updated

3 years ago
Duplicate of this bug: 1286282

Comment 42

3 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

3 years ago
Status: RESOLVED → VERIFIED
Reporter

Updated

3 years ago
See Also: → 1327657
You need to log in before you can comment on or make changes to this bug.