771348 - Work - Fix SecurityChange events so that they will fire for SSL-encrypted pages

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: App Bar, defect)

Description

[Jonathan Wilde [:jwilde]]

On the current elm tip, SecurityChange events that would cause the browser to identify the current site as SSL (EV or not) don't fire.

As a result, when you browse to an encrypted page and tap the identity icon, the UI incorrectly shows that there's no identifying information for the current site.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

Background:
  toolkit/content/widgets/browser.xml defines a property "securityUI" whose getter is responsible for instantiating a new "@mozilla.org/secure_browser_ui;1", initializing it, and setting it as a property of the docshell [1].  This getter is invoked from the constructor that is also defined in toolkit/content/widgets/browser.xml [2].

The problem:
  browser/metro/base/content/bindings/browser.xml ALSO defines a "securityUI" property, and that property's getter does none of the initialization that the other "securityUI" property does [3].  When the constructor in toolkit/content/widgets/browser.xml calls the securityUI getter, the getter in browser/metro/base/content/bindings/browser.xml is invoked instead of the one in toolkit/content/widgets/browser.xml.  As a result, the "@mozilla.org/secure_browser_ui;1" is never created, and the docshell does not have a securityUI property.

This patch fixes the issue by removing the securityUI property from toolkit/content/widgets/browser.xml.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml?rev=f926fbeab6af#617
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml?rev=f926fbeab6af#714
[3] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bindings/browser.xml?rev=b05237492a4c#57

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Note: The patch for this bug doesn't have any visible effect until you also apply the patch in bug 801090

Comment 3

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes (:TimAbraldes) from comment #1)
> This patch fixes the issue by removing the securityUI property from
> toolkit/content/widgets/browser.xml.

Did you mean browser/metro/base/content/bindings/browser.xml ? :)

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Jim Mathies [:jimm] from comment #3)
> (In reply to Tim Abraldes (:TimAbraldes) from comment #1)
> > This patch fixes the issue by removing the securityUI property from
> > toolkit/content/widgets/browser.xml.
> 
> Did you mean browser/metro/base/content/bindings/browser.xml ? :)

Good catch, Jim.  Just making sure that everyone who is reading along is paying attention ;)

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 748333 [details] [diff] [review]
Patch v1

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

::: browser/metro/base/content/bindings/browser.xml
@@ +498,5 @@
> +	      if (!currentURI) {
> +	        result.isException = false;
> +              } else {
> +                result.isException = !!this._overrideService.hasMatchingOverride(currentURI.asciiHost, currentURI.port, cert, {}, {});
> +	      }

Minor nits: Remove the "!!" while you're here; it's not needed.

I think I'd find this slightly clearer in the opposite order:

  if (currentURI) {
    // blah blah blah
  } else {
    result.isException = false;
  }

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

Review comments addressed

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf2bbb3243f3

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf2bbb3243f3