1276117 - Url stays in location bar when I open homepage

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160522030240
STR_1:
1. Open open Australis menu -> click "Options" barbutton, wait until preferences page loads
2. Click "Home" toolbarbutton

AR:  "about:preferences" stays in urlbar
ER:  Url should disappear

This is regression from bug 1241085. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=553a9575ad40687f2a5486b1170de0ae8a280015&tochange=e4260b4769a02960afa8ffe89cf1824fa0580a30

Comment 1

[Marco Bonardo [:mak]]

Gijs, any ideas?

Comment 2

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #1)
> Gijs, any ideas?

Not off-hand. I'll have to have a closer look (keeping needinfo), but it seems to be specific to loading non-remote pages one after another using something other than the location bar, and particularly the case where the new value would normally cause the URL bar to be cleared completely (rather than replaced with something else). So e.g. loading a bookmark for about:home as step (2) here works, but loading a bookmark for about:debugging (also parent process) doesn't. Doesn't happen for remote URIs (like http URIs stored in bookmarks) either. So it's a pretty specific case.

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> So e.g. loading a bookmark for
> about:home as step (2) here works, but loading a bookmark for
> about:debugging (also parent process) doesn't.

Eh, to be clear, when I say "works", I mean it reproduces the same problem as comment #0, and that same problem does not reproduce with a bookmark for e.g. about:debugging.

Comment 4

[:Gijs (he/him)]

In fact, it's also required that the initial page you open passes through addTab() which has a hack that sets userTypedValue on the tab to the URI we're opening in that tab... Sigh. :-(

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> In fact, it's also required that the initial page you open passes through
> addTab() which has a hack that sets userTypedValue on the tab to the URI
> we're opening in that tab... Sigh. :-(

Hm, no, that's actually wrong - the same thing happens if you open a new tab first and we reuse that tab. :-\

Comment 6

[arni2033]

Remaining scenarios:

STR_2:
 1. Open new window, open new tab page in that window
 2. Close and restore that window
 3. Switch to pending tab with "about:home"
AR:  Url "about:home" promptly appears in urlbar

STR_3:
 1. Open new tab page
 2. Type "about:home" in urlbar, press Enter
AR:  "about:home" stays in urlbar

STR_4:
 1. Open new window
 2. Type "about:newtab" in urlbar, press Enter
 3. Press Alt+Left, then Alt+Right several times
AR:  Both urls promptly appear in urlbar producing extreme text twitching.

Comment 7

[:Gijs (he/him)]

And I'm also wrong in my description of the issue, because we load about:home in the content process. Doesn't happen with other remote (content process) pages though, so there's something fishy going on. The string in question is restored here: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#787 after the remoteness switch from non-remote to remote browser that occurs when loading about:home.

(In reply to arni2033 from comment #6)
> Remaining scenarios:
> 
> STR_2:
>  1. Open new window, open new tab page in that window
>  2. Close and restore that window
>  3. Switch to pending tab with "about:home"
> AR:  Url "about:home" promptly appears in urlbar
> 
> STR_3:
>  1. Open new tab page
>  2. Type "about:home" in urlbar, press Enter
> AR:  "about:home" stays in urlbar
> 
> STR_4:
>  1. Open new window
>  2. Type "about:newtab" in urlbar, press Enter
>  3. Press Alt+Left, then Alt+Right several times
> AR:  Both urls promptly appear in urlbar producing extreme text twitching.

Not sure all of these are caused by the same issue or not, but I'll file followups myself if they're not.

Comment 8

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1276117 - part 1: fix URL bar state when loading about:home after about:preferences, r?mikedeboer

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

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1276117 - part 2: fix the URL bar state when loading about:home from a new tab page, r?mikedeboer

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

Comment 10

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly in the URL bar while loading, r?mikedeboer

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

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to arni2033 from comment #6)
> > AR:  Url "about:home" promptly appears in urlbar

For future reference, 'promptly' means 'immediately' or 'right now', not 'briefly' / 'for a short period of time', which I think is what you mean. I wasted a lot of time not understanding what this was about. Anyway, patches up now.

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8757903 [details]
MozReview Request: Bug 1276117 - part 2: fix the URL bar state when loading about:home from a new tab page, r?mikedeboer

https://reviewboard.mozilla.org/r/56266/#review52908

::: browser/base/content/tabbrowser.xml:648
(Diff revision 1)
>                      // we loaded off a blank browser and this is an initial page load
>                      // (e.g. about:privatebrowsing, about:newtab, etc.) so we avoid
>                      // clearing the location bar in case the user is typing in it.
>                      // loading about:blank shouldn't trigger this, either, because its
> -                    // loads are "special".
> +                    // loads are "special". Finally, we *do* do it if we load such a URL
> +                    // at the explicit (URL bar-based) request of the user.

This comment explains the if-statement above in-order, so can you put your comment in the right place?
Or perhaps it's more fitting to move your addition inside the if-statement to the end.

::: browser/base/content/urlbarBindings.xml:386
(Diff revision 1)
>            function continueOperation()
>            {
>              this.value = url;
>              gBrowser.userTypedValue = url;
> +            if (gInitialPages.includes(url)) {
> +              gBrowser.selectedBrowser.explicitInitialURLLoad = url;

What does 'explicit' mean here?

Comment 13

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Comment on attachment 8757903 [details]
> MozReview Request: Bug 1276117 - part 2: fix the URL bar state when loading
> about:home from a new tab page, r?mikedeboer
> 
> https://reviewboard.mozilla.org/r/56266/#review52908
> 
> ::: browser/base/content/tabbrowser.xml:648
> (Diff revision 1)
> >                      // we loaded off a blank browser and this is an initial page load
> >                      // (e.g. about:privatebrowsing, about:newtab, etc.) so we avoid
> >                      // clearing the location bar in case the user is typing in it.
> >                      // loading about:blank shouldn't trigger this, either, because its
> > -                    // loads are "special".
> > +                    // loads are "special". Finally, we *do* do it if we load such a URL
> > +                    // at the explicit (URL bar-based) request of the user.
> 
> This comment explains the if-statement above in-order, so can you put your
> comment in the right place?

It doesn't, actually (explain in the order of the if-statement). I can fix it so it does, but I expect it will read more clumsily...

> Or perhaps it's more fitting to move your addition inside the if-statement
> to the end.

That modifies blame more, and the condition fits better with the first few checks than the last.

> > +              gBrowser.selectedBrowser.explicitInitialURLLoad = url;
> 
> What does 'explicit' mean here?

explicitly typed (and 'hit enter on') by the user rather than by virtue of them having opened a new (private) window, new tab, crashed, etc.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8757903 [details]
MozReview Request: Bug 1276117 - part 2: fix the URL bar state when loading about:home from a new tab page, r?mikedeboer

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

Comment 15

[:Gijs (he/him)]

Comment on attachment 8757904 [details]
MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly in the URL bar while loading, r?mikedeboer

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

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8757902 [details]
MozReview Request: Bug 1276117 - part 1: fix URL bar state when loading about:home after about:preferences, r?mikedeboer

https://reviewboard.mozilla.org/r/56264/#review52920

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8757903 [details]
MozReview Request: Bug 1276117 - part 2: fix the URL bar state when loading about:home from a new tab page, r?mikedeboer

https://reviewboard.mozilla.org/r/56266/#review52922

Thanks for the clarification!

::: browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js:53
(Diff revision 2)
> + */
> +add_task(function* clearURLBarAfterManuallyLoadingAboutHome() {
> +  let tabOpenedAndSwitchedTo = BrowserTestUtils.switchTab(gBrowser, () => {});
> +  // This opens about:newtab:
> +  BrowserOpenTab();
> +  let tab = yield tabOpenedAndSwitchedTo;

I like the `promiseTabOpenedAndSwitched` convention that reads like something is about to happen.
Your call if you want to change it here.

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8757904 [details]
MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly in the URL bar while loading, r?mikedeboer

https://reviewboard.mozilla.org/r/56268/#review52912

I am getting this new assertion hit in docShell. I'm betting it's not a blocker for this bug, but I just wanted to mention it here for posterity.

27 INFO TEST-PASS | browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js | URL bar value should stay empty. -
[Child 19128] ###!!! ASSERTION: no SHEntry for a non-transient viewer?: 'NS_IsAboutBlank(mCurrentURI)', file /Users/mikedeboer/Projects/fx-team/docshell/base/nsDocShell.cpp, line 11447
#01: nsDocShell::OnLoadingSite(nsIChannel*, bool, bool) (nsDocShell.cpp:11604, in XUL)
#02: nsDocShell::CreateContentViewer(nsACString_internal const&, nsIRequest*, nsIStreamListener**) (nsDocShell.cpp:9065, in XUL)
#03: nsDSURIContentListener::DoContent(nsACString_internal const&, bool, nsIRequest*, nsIStreamListener**, bool*) (nsDSURIContentListener.cpp:129, in XUL)
#04: nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) (nsURILoader.cpp:720, in XUL)
#05: nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) (nsURILoader.cpp:398, in XUL)
#06: nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) (nsURILoader.cpp:259, in XUL)
#07: nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) (nsBaseChannel.cpp:807, in XUL)
#08: nsInputStreamPump::OnStateStart() (nsInputStreamPump.cpp:525, in XUL)
#09: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) (nsInputStreamPump.cpp:427, in XUL)
#10: nsInputStreamReadyEvent::Run() (nsCOMPtr.h:375, in XUL)
#11: nsThread::ProcessNextEvent(bool, bool*) (nsCOMPtr.h:403, in XUL)
#12: NS_ProcessPendingEvents(nsIThread*, unsigned int) (nsThreadUtils.cpp:232, in XUL)
#13: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:98, in XUL)
#14: nsAppShell::ProcessGeckoEvents(void*) (nsAppShell.mm:388, in XUL)
#15: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (in CoreFoundation) + 17
#16: __CFRunLoopDoSources0 (in CoreFoundation) + 556
#17: __CFRunLoopRun (in CoreFoundation) + 927
#18: CFRunLoopRunSpecific (in CoreFoundation) + 296
#19: RunCurrentEventLoopInMode (in HIToolbox) + 235
#20: ReceiveNextEventCommon (in HIToolbox) + 432
#21: _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 71
#22: _DPSNextEvent (in AppKit) + 1067
#23: -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] (in AppKit) + 454
#24: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (nsAppShell.mm:121, in XUL)
#25: -[NSApplication run] (in AppKit) + 682
#26: nsAppShell::Run() (nsCOMPtr.h:536, in XUL)
#27: XRE_RunAppShell (nsEmbedFunctions.cpp:827, in XUL)
#28: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (MessagePump.cpp:285, in XUL)
#29: MessageLoop::Run() (message_loop.cc:493, in XUL)
#30: XRE_InitChildProcess (nsAutoPtr.h:199, in XUL)
#31: content_process_main(int, char**) (plugin-container.cpp:231, in plugin-container)

::: browser/base/content/browser.js:6427
(Diff revision 1)
>      if (uri.spec == "about:blank" && contentPrincipal.isNullPrincipal) {
>        return true;
>      }
> +    // Initial browser loads of URIs when we've changed remoteness means
> +    // contentPrincipal hasn't updated.
> +    if (contentPrincipal.URI.spec == "about:blank") {

Can you coalesce this with the if-statement above? It already has the same subject.

Comment 19

[arni2033]

(In reply to :Gijs Kruitbosch from comment #11)
> For future reference, 'promptly' means 'immediately' or 'right now', not
> 'briefly' / 'for a short period of time', which I think is what you mean. I
> wasted a lot of time not understanding what this was about.
I'm very sorry about that! At some point, I started using a wrong word.   v_v

Also, you were right: STR_2 and STR_4 were not caused by bug 1241085. STR_5 was. (I'm quite busy right
now, so I can't download builds to test STR_2 and STR_4. Please open a follow-up for STR_5 if needed)
STR_5:
 1. Press Ctrl+T to open a new tab
 2. Type "asdf" in urlbar
 3. Press Alt+Home
AR: "asdf" stays in urlbar
ER: "asdf" should disappear. Reference: it works this way in private window

Comment 20

[:Gijs (he/him)]

(In reply to arni2033 from comment #19)
> (In reply to :Gijs Kruitbosch from comment #11)
> > For future reference, 'promptly' means 'immediately' or 'right now', not
> > 'briefly' / 'for a short period of time', which I think is what you mean. I
> > wasted a lot of time not understanding what this was about.
> I'm very sorry about that! At some point, I started using a wrong word.   v_v
> 
> Also, you were right: STR_2 and STR_4 were not caused by bug 1241085. STR_5
> was. (I'm quite busy right
> now, so I can't download builds to test STR_2 and STR_4. Please open a
> follow-up for STR_5 if needed)
> STR_5:
>  1. Press Ctrl+T to open a new tab
>  2. Type "asdf" in urlbar
>  3. Press Alt+Home
> AR: "asdf" stays in urlbar
> ER: "asdf" should disappear. Reference: it works this way in private window

OK, but at this stage there are r+'d patches on the bug that I'll land, and they address STR 2 and 4. I'll look at STR 5 tomorrow if these patches do not already address it (not sure at this stage, but I suspect not). I'm... not sure to what degree they can be addressed without regressing bug 1241085. There's not really a way for the code in question to know whether you're loading about:home (or about:privatebrowsing, or about:newtab, ...) manually (you clicked something, you put it in the location bar, whatever) or if the browser is loading it because you opened a new window / new tab / new private window.

A lot of this will go away when about:newtab will be made to load in the child process, so I'm also not sure how valuable it is to invest time in these issues.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd046e409fb
part 1: fix URL bar state when loading about:home after about:preferences, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/34ef5d33979d
part 2: fix the URL bar state when loading about:home from a new tab page, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8945f94a99
part 3: fix 'about:home' appearing briefly in the URL bar while loading, r=mikedeboer

Comment 22

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #20)
> (In reply to arni2033 from comment #19)
> > (In reply to :Gijs Kruitbosch from comment #11)
> > > For future reference, 'promptly' means 'immediately' or 'right now', not
> > > 'briefly' / 'for a short period of time', which I think is what you mean. I
> > > wasted a lot of time not understanding what this was about.
> > I'm very sorry about that! At some point, I started using a wrong word.   v_v
> > 
> > Also, you were right: STR_2 and STR_4 were not caused by bug 1241085. STR_5
> > was. (I'm quite busy right
> > now, so I can't download builds to test STR_2 and STR_4. Please open a
> > follow-up for STR_5 if needed)
> > STR_5:
> >  1. Press Ctrl+T to open a new tab
> >  2. Type "asdf" in urlbar
> >  3. Press Alt+Home
> > AR: "asdf" stays in urlbar
> > ER: "asdf" should disappear. Reference: it works this way in private window
> 
> OK, but at this stage there are r+'d patches on the bug that I'll land, and
> they address STR 2 and 4. I'll look at STR 5 tomorrow if these patches do
> not already address it (not sure at this stage, but I suspect not). I'm...
> not sure to what degree they can be addressed without regressing bug
> 1241085. There's not really a way for the code in question to know whether
> you're loading about:home (or about:privatebrowsing, or about:newtab, ...)
> manually (you clicked something, you put it in the location bar, whatever)
> or if the browser is loading it because you opened a new window / new tab /
> new private window.
> 
> A lot of this will go away when about:newtab will be made to load in the
> child process, so I'm also not sure how valuable it is to invest time in
> these issues.

Filed bug 1277060.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bd046e409fb
https://hg.mozilla.org/mozilla-central/rev/34ef5d33979d
https://hg.mozilla.org/mozilla-central/rev/0e8945f94a99

Comment 24

[:Gijs (he/him)]

Comment on attachment 8757904 [details]
MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly in the URL bar while loading, r?mikedeboer

Approval Request Comment
[Feature/regressing bug #]: bug 1241085 & co
[User impact if declined]: confusing URL bar state in some edgecases
[Describe test coverage new/current, TreeHerder]: extensive test coverage for bug 1241085, and tests in this patch-set
[Risks and why]: reasonably low because of the automated test coverage
[String/UUID change made/needed]: no.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8757904 [details]
MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly in the URL bar while loading, r?mikedeboer

Has good test coverage, fix for recent regression, let's uplift it.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking fix-optional for beta; if you want this on beta please request uplift.

Comment 27

[Dão Gottwald [:dao]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #25)
> Comment on attachment 8757904 [details]
> MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly
> in the URL bar while loading, r?mikedeboer
> 
> Has good test coverage, fix for recent regression, let's uplift it.

Looks like this slipped through.

Comment 28

[:Gijs (he/him)]

Comment on attachment 8757904 [details]
MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly in the URL bar while loading, r?mikedeboer

Thanks, Dão.

Approval Request Comment
[Feature/regressing bug #]: bug 1241085
[User impact if declined]: in some cases the URL bar flickers or displays the wrong value
[Describe test coverage new/current, TreeHerder]: comes with tests
[Risks and why]: reasonably low, has baked for a 3+ weeks on nightly + aurora without known regressions
[String/UUID change made/needed]: no.

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8757904 [details]
MozReview Request: Bug 1276117 - part 3: fix 'about:home' appearing briefly in the URL bar while loading, r?mikedeboer

This patch fixes a regression. Take it in 48 beta 7.

Comment 30

[Gerry Chang [:gchang]]

Hi :Gijs,
Per beta uplift request, is part 3 the only patch or are all the parts (1~3) uplift requests?

Comment 31

[Carsten Book [:Tomcat]]

also its already on aurora with the merge day

Comment 32

[:Gijs (he/him)]

(In reply to Carsten Book [:Tomcat] from comment #31)
> also its already on aurora with the merge day

The problem is the aurora uplift was for 48, not for 49, and so now it still hasn't made 48.

(In reply to Gerry Chang [:gchang] from comment #30)
> Hi :Gijs,
> Per beta uplift request, is part 3 the only patch or are all the parts (1~3)
> uplift requests?

All the patches should be uplifted.

Comment 33

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/78ceb0e9fde2
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/58db190f800e
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/75b9cc8259df

Comment 34

[Tanvir Rahman]

I have reproduced this bug with nightly 49.0a1 (2016-05-26) on Windows 10, 64 bit!

The Bug's fix is now verified on Latest Beta 48.0b6, Aurora 49.0a2-

Beta 48.0b6
Build ID 	20160706215822
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

Aurora 49.0a2
Build ID 	20160708004052
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160708]