908534 - Opening one of the "about" pages in a new tab does not use the correct identity block

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Sean Smith]

In current Nightly, if you use Firefox Button -> Help -> Nightly Health Report, the health report opens in a new tab and the identity block does not indicate that you are on a "secure Nightly page". But, if you switch to a different tab and then come back, the identity block changes and clicking the globe shows that you are on a secure Nightly page.

Opening a new tab and typing about:healthreport in the address bar and hitting enter, the identity block correctly shows the page as being secure. Opening it from a bookmark in the current tab, the identity block shows correctly. Opening from a bookmark into a new tab and the identity block is wrong.

So, it seems to be triggered by opening into a new tab.

Seen in Win7 and Win8, though I suspect it is platform ALL.

In the event I can't figure out how to add the dependency, this should block bug 750106 and have tracking flags for Firefox 25/26.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Jared this looks to be yours :)

Comment 2

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

For some reason, the spec is coming in as about:blank for these pages when they are opened in a new tab. I think this is because we load about:blank and then load the eventual page, so maybe this is a race condition?

The easiest fix (and more like a band-aid), is to just start showing the chrome-identity on about:blank pages.

Comment 3

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

The bug appears to be right here, http://hg.mozilla.org/mozilla-central/annotate/14b1e8c2957e/browser/base/content/browser.js#l3972

Comment 4

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

Dao, what do you think is going on here?

When I set a breakpoint on the next line, uri.spec is "about:blank", however if I check gBrowser.currentURI.spec at that point it reads back as "about:healthreport". Hency my race condition hypothesis.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #4)
> Dao, what do you think is going on here?
> 
> When I set a breakpoint on the next line, uri.spec is "about:blank", however
> if I check gBrowser.currentURI.spec at that point it reads back as
> "about:healthreport".

That doesn't make any sense to me.

Comment 6

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

Hah, well it doesn't make much sense to me either! :)

Comment 7

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

Do you just mean that you see different behavior if you dump() the value than if you break on that line and inspect the value? The breakpoint may just be interfering with timing of the loads in the browser (or just delaying your inspection until the value changes).

Comment 8

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Do you just mean that you see different behavior if you dump() the value
> than if you break on that line and inspect the value? The breakpoint may
> just be interfering with timing of the loads in the browser (or just
> delaying your inspection until the value changes).

I mean that I see different values within the debugger depending on what line I have paused execution on.

Comment 9

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

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #7)
> Do you just mean that you see different behavior if you dump() the value
> than if you break on that line and inspect the value? The breakpoint may
> just be interfering with timing of the loads in the browser (or just
> delaying your inspection until the value changes).

If I place dump() statements here I get the same value for all dump statements (about:blank).

Comment 10

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

Placing a dump in onLocationChange and onSecurityChange shows the following:
onSecurityChange: about:blank
onLocationChange: about:healthreport

Comment 11

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #6)
> Hah, well it doesn't make much sense to me either! :)

The new debugger API spins the event loop, so it's possible that other code executes in the background, which would have changed currentURI even though you were paused. This can lead to confusing situations like here when it comes to race conditions. The best way to be sure is dump things in the code, or use a carefully constructed conditional breakpoint that captures the situation you care about. :-(

Comment 12

[Alex Keybl [:akeybl]]

jaws, when are you targeting resolving? Or would you like to make the case for this not being critical enough to track?

Comment 13

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

I'd like to get this fixed, but each time I look at it I don't get much further. With that being said, I don't think this is a high severity bug and as such I don't think we should be worried about this being released.

Comment 17

[Peter Retzer (:pretzer)]

Should this be tracked for Australis, since the visibility of this bug is increased with the new customization mode identity block? (see bug 964206)

Comment 18

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

I don't think so. It's not an Australis-specific regression, and the effect of the identity block hiding/showing when in customization mode is quite subtle.

We have other obvious UI changes in customization mode to let the user know that this isn't spoofed (such as the increased padding on the sides of the window), and the location bar is shown as a "blank slate" during customization mode since users are moving the generic location bar at that time.

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

The problem here seems to be that the docShell checks whether it can load a URI and thus the MixedContentBlocker starts to call OnSecurityChange() listeners. This however happens even before the internal load actually starts and OnLocationChange() listeners are called.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Updating the security UI in onLocationChange() would work, it however means a lot more updates than actually necessary and also a some flickering due to a slight delay before the URI load starts. So, not a great solution.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> The problem here seems to be that the docShell checks whether it can load a
> URI and thus the MixedContentBlocker starts to call OnSecurityChange()
> listeners. This however happens even before the internal load actually
> starts and OnLocationChange() listeners are called.

Sounds like this isn't a front-end bug then.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

This might as well be a front-end bug if we think that announcing a security change before a location change is ok and the front-end has to handle that. about: pages are weird in the sense that they can change the current security state before the page even started to load.

Comment 24

[Alice0775 White]

Now, preferences page(Bug 738797) also is affected this bug.

Comment 26

[Benjamin Kerensa [:bkerensa]]

Jared, Has your position changed on this? Do you think we need to track this for 32?

Comment 27

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

in-content-prefs aren't going to make the 32 cutoff. this bug will be important to get fixed when we do ship in-content prefs, which we should track for firefox-33. so in summary, this shouldn't be tracked for firefox 32.

Comment 28

[:Gijs (he/him)]

I can poke at it this iteration. Marco, can you add this to the backlog?

Comment 29

[Marco Mucci [:MarcoM]]

Added to Iteration 33.1

Gijs, would this bug be a [qa-] or [qa+] for verification?

Comment 30

[:Gijs (he/him)]

(In reply to Tim Taubert [:ttaubert] from comment #20)
> The problem here seems to be that the docShell checks whether it can load a
> URI and thus the MixedContentBlocker starts to call OnSecurityChange()
> listeners. This however happens even before the internal load actually
> starts and OnLocationChange() listeners are called.

I'm confused. When I log happenings from the onSecurityChange listener, I only see the call triggered by setting the selected tab. Are you saying this event gets lost because the tabbrowser hasn't gotten its hands on the tab at the point where that event is fired?

Comment 31

[:Gijs (he/him)]

Sooo... here's what I see happening.

On the JS side, the only time onSecurityChange fires for the opening of about:healthreport in a new tab, it's because tabbrowser.xml's selectedTab setter calls a bunch of the progress listeners. At that point the relevant URL is about:blank and we clear any previous state.

Unfortunately that's the only time we get called.

On the C++ side, first we get a call that might lead to notifications from the start of the request. The top of the stack looks like:

#0      0x0000000102aa6b86 in nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState(nsIRequest*, nsISupports*, bool) at security/manager/boot/src/nsSecureBrowserUIImpl.cpp:470
#1      0x0000000102aa7f24 in nsSecureBrowserUIImpl::OnLocationChange(nsIWebProgress*, nsIRequest*, nsIURI*, unsigned int) at security/manager/boot/src/nsSecureBrowserUIImpl.cpp:1448
#2      0x0000000101647ffa in nsDocLoader::FireOnLocationChange(nsIWebProgress*, nsIRequest*, nsIURI*, unsigned int) at uriloader/base/nsDocLoader.cpp:1281
#3      0x00000001029a4075 in nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**) at docshell/base/nsDocShell.cpp:8408
#4      0x00000001029b2a6a in nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*) at docshell/base/nsDSURIContentListener.cpp:122
#5      0x00000001016499e6 in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) at uriloader/base/nsURILoader.cpp:710
#6      0x0000000101648f3b in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) at uriloader/base/nsURILoader.cpp:386
#7      0x0000000101648d30 in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) at uriloader/base/nsURILoader.cpp:262
#8      0x00000001010da6ba in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) at netwerk/base/src/nsBaseChannel.cpp:715
#9      0x00000001010e7129 in nsInputStreamPump::OnStateStart() at netwerk/base/src/nsInputStreamPump.cpp:521
#10     0x00000001010e6ec9 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) at netwerk/base/src/nsInputStreamPump.cpp:433

We then decide there's a change of security context and we should notify things.

Unfortunately, we then end up at: http://hg.mozilla.org/mozilla-central/annotate/75377bac231e/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#l1358

with a null temp_ToplevelEventSink which means we don't notify anyone. That's unfortunate, because we then get called again with this stack:

#0      0x0000000102aa6b86 in nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState(nsIRequest*, nsISupports*, bool) at security/manager/boot/src/nsSecureBrowserUIImpl.cpp:470
#1      0x0000000102aa78de in nsSecureBrowserUIImpl::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, tag_nsresult) at security/manager/boot/src/nsSecureBrowserUIImpl.cpp:1160
#2      0x00000001016470e8 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, tag_nsresult) at uriloader/base/nsDocLoader.cpp:1268
#3      0x0000000101646bd8 in nsDocLoader::FireOnStateChange(nsIWebProgress*, nsIRequest*, int, tag_nsresult) at uriloader/base/nsDocLoader.cpp:1231
#4      0x0000000101646a1e in nsDocLoader::doStopURLLoad(nsIRequest*, tag_nsresult) [inlined] at uriloader/base/nsDocLoader.cpp:807
#5      0x0000000101646a04 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) at uriloader/base/nsDocLoader.cpp:613
#6      0x0000000101646c3a in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) at uriloader/base/Unified_cpp_uriloader_base0.cpp:628
#7      0x00000001010e8875 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) at netwerk/base/src/nsLoadGroup.cpp:689
#8      0x00000001010daa7b in nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) at netwerk/base/src/nsBaseChannel.cpp:739
#9      0x00000001010daaba in non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) at netwerk/base/src/Unified_cpp_netwerk_base_src1.cpp:746
#10     0x00000001010e74f1 in nsInputStreamPump::OnStateStop() at netwerk/base/src/nsInputStreamPump.cpp:711
#11     0x00000001010e6f1f in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) at netwerk/base/src/nsInputStreamPump.cpp:440

This time, when we hit UpdateSecurityState at http://hg.mozilla.org/mozilla-central/annotate/75377bac231e/security/manager/boot/src/nsSecureBrowserUIImpl.cpp#l1256, it decides that no flags have changed, the location isn't new and the status isn't updated, so... it doesn't call TellTheWorld at all.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

This is also caused by bug 878747. Not quite a regression but still good to know where the null event sink comes from. Loading about:blank and stopping that right away seems to at least ensure that we have a top level event sink.

Comment 33

[:Gijs (he/him)]

Attachment: change of event sink should trigger OnSecurityChange notifications,

So as I said on IRC, OnStateChange actually also calls EvaluateAndUpdateSecurityState, but it doesn't pass on the fact that sinkChanged is now true. This patch changes that. It fixes the case as reported in comment #0. I'll create a separate frontend browser test to test this case.

Comment 34

[:Gijs (he/him)]

Attachment: test that chromeUI notification is shown for about:support when opened as a new tab,

This fails before, and works after applying both the fix from here and the nitpick from bug 941487 because mochitest doesn't like uncaught exceptions. Sigh.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8438357 [details] [diff] [review]
test that chromeUI notification is shown for about:support when opened as a new tab,

Review of attachment 8438357 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser_aboutSupport_newtab_security_state.js
@@ +13,5 @@
> +    disableMCB: false
> +  });
> +
> +  tab.linkedBrowser.addEventListener("load", function onload() {
> +    if (gBrowser.currentURI.spec == "about:blank") {

Using promiseBrowserLoaded() as defined in sessionstore/test/head.js and add_task() would make the test a little nicer, I think. Doesn't make it any more correct though. Your call.

Comment 36

[Dão Gottwald [:dao]]

Comment on attachment 8438357 [details] [diff] [review]
test that chromeUI notification is shown for about:support when opened as a new tab,

>+    tab = null;

What is this good for?

Comment 37

[:Gijs (he/him)]

Attachment: test that chromeUI notification is shown for about:support when opened as a new tab,

Comment 38

[:Gijs (he/him)]

Comment on attachment 8438449 [details] [diff] [review]
test that chromeUI notification is shown for about:support when opened as a new tab,

Carrying over review.

Comment 39

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8438340 [details] [diff] [review]
change of event sink should trigger OnSecurityChange notifications,

Review of attachment 8438340 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with nits addressed.

::: security/manager/boot/src/nsSecureBrowserUIImpl.cpp
@@ +454,5 @@
>  }
>  
>  nsresult
>  nsSecureBrowserUIImpl::EvaluateAndUpdateSecurityState(nsIRequest* aRequest, nsISupports *info,
> +                                                      bool withNewLocation, bool withNewSink)

nit: would you mind wrapping this line (and the one above) at 80 characters?

@@ +512,5 @@
>      // document load finishes.
>      mRestoreSubrequests = false;
>    }
>  
> +  return UpdateSecurityState(aRequest, withNewLocation, withNewSink || updateStatus);

nit: wrap to 80 chars

Comment 40

[:Gijs (he/him)]

w/ nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/dfe571731a0d
remote:   https://hg.mozilla.org/integration/fx-team/rev/2bd196dc1b2c

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dfe571731a0d
https://hg.mozilla.org/mozilla-central/rev/2bd196dc1b2c

Comment 42

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Windows 8 32bit, Ubuntu 13.10 32bit and Mac OSX 10.8.5 using latest Nightly 33.0a1 (buildID: 20140615030204).