889428 - Switching tabs from a Nightly/UX tab to a Nightly/UX tab and then changing the URL maintains the Nightly/UX icon.

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Suraj Patel]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130702 Firefox/24.0 (Nightly/Aurora)
Build ID: 20130702004019

Steps to reproduce:

1. Start a new browsing session.
2. Change the tab URL to about:home so that the Nightly/UX indicator is shown in the address bar.
3. Open a new tab and again navigate to about:home so that the Nightly/UX indicator is shown in the address bar.
4. Switch back to the first tab and change the URL to any normal web page.

Note that about:home could be any about: page which adds the indicator.


Actual results:

Nightly/UX page indicator remains.


Expected results:

It should disappear.

Comment 1

[:Gijs (he/him)]

I can reproduce this on Windows Nightly, but couldn't on OS X earlier today. Not sure why. Either way, confirming.

Comment 2

[Matthew N. [:MattN]]

Marking for tracking as this can make a website appear as trusted chrome UI.

Comment 3

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

I think this is a regression from bug 398360 actually, it's just that chromeUI wasn't introduced until bug 590206, and even at that it wasn't obvious when it was wrong until the patches for bug 750106 landed.

Comment 4

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

And https://bugzilla.mozilla.org/show_bug.cgi?id=398360#c6 mentions that onLocationChange won't be fired for error pages, which are the same as chrome pages as I understand it.

Comment 5

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

In the case where this is failing, onSecurityChange is being called before onLocationChange, but there is an implicit ordering dependency between these functions since onLocationChange resets this._hostChanged and onSecurityChange checks this value.

A simple patch here would be to only set this._hostChanged=false when the page is not a chrome URI, although that seems hacky.

I'm currently working on getting a mochitest that reproduces this broken ordering, but when I run this through mochitest the events appear in the order of onLocationChange then onSecurityChange. When I run these STR manually, I see onSecurityChange then onLocationChange.

Comment 6

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

Attachment: Wallpaper patch

Comment 7

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

Attachment: Tests

Tests that pass without the patch applied, so something here isn't right...

Comment 8

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

Attachment: Tests (qref'd)

Comment 9

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

Attachment: Tests v2

This test reproduces the STR that were given in comment #0. It also demonstrates bug 882977.

Comment 10

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

Comment on attachment 770659 [details] [diff] [review]
Wallpaper patch

mach mochitest-browser browser/base/content/test
 2:34.98 INFO TEST-START | Shutdown
 2:34.98 Browser Chrome Test Summary
 2:34.98        Passed: 6147
 2:34.98        Failed: 0
 2:34.98        Todo: 10

with the accompanied test applied. Without this patch the accompanied test fails.

Comment 11

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

(In reply to Suraj Patel from comment #0)

Thank you so very much for these detailed steps to reproduce. This was a very well written bug report. Welcome to Bugzilla! :-)

Comment 12

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

(In reply to Jared Wein [:jaws] from comment #4)
> And https://bugzilla.mozilla.org/show_bug.cgi?id=398360#c6 mentions that
> onLocationChange won't be fired for error pages, which are the same as
> chrome pages as I understand it.

I should point out that this assumption is likely wrong, as error documents likely replace the current document but keep the location the same.

Comment 13

[Alex Keybl [:akeybl]]

We typically do not track security regressions less than sec-high/critical. We would take a low risk uplift if necessary though.

Comment 14

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

Attachment: Tests v2.1

Noticed some debugging code that should have been removed.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 770659 [details] [diff] [review]
Wallpaper patch

The whole _hostChanged thingy seems broken. I think we should fix that, not wallpaper over it.

Comment 16

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

Comment on attachment 770659 [details] [diff] [review]
Wallpaper patch

Made obsolete by the patch in bug 893424.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 774750 [details] [diff] [review]
Tests v2.1

>diff --git a/browser/base/content/test/browser_bug889428.js b/browser/base/content/test/browser_bug889428.js

>+function loadURIInSelectedTab(aURL, aCallback) {

>+  gNewWin.gBrowser.selectedBrowser.addEventListener("load", function() {

>+    gNewWin.gBrowser.selectedBrowser.removeEventListener("load", arguments.callee, true);

Use a named function rather than arguments.callee.

>+function test() {

>+  let oldTab = gBrowser.selectedTab;

unused?

The whole gNewWin/loadURIInSelectedTab/firstTab setup seems confusing, it seems like you could refactor this so that what's it's actually doing is a bit clearer (use a closure to avoid the need for gNewWin, avoid having helpers that operate on selectedTab and instead use explicit references to the relevant tabs, etc.).

Comment 18

[Matthew N. [:MattN]]

Any reason this was still open?

Comment 19

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

It was just open for the test that was here. There hasn't been much progress on the test and I won't be having any time for it in the near future either, so it's fine to close this.