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.

Jared this looks to be yours :)

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.

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

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.

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

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

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

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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).

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

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

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

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.

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

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.

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.

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.

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

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.

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

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

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.

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

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

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

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.

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.

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.

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

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

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

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