571868 - Pull identity data from message data

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

A web page's identity data is now collected in a frame script and sent to the UI using JSON. This patch changes the front-end code to use the JSON data to populate the identity information.

The JSON is created in bindings/browser.js in the WebProgress:SecurityChange message.

Tested OK using normal pages, addons.mozilla.org and bugzilla.mozilla.org. Also tested switching and closing tabs. The identity info was updated correctly.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip v0.1

I think this won't work because you need to call securityUI.init(content) somewhere to turn on the security notifications. This probably works on mozilla-central because we don't redefine securityUI but i'm pretty sure this doesn't work on e10s.

This explain why my patch was so different from yours!
It is not final (it has some problems when you open a new tab as example) but pretty close to final i think

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

> +  receiveMessage: function(aMessage) {
> +    const SECUREBROWSERUI_CONTRACTID = "@mozilla.org/secure_browser_ui;1";
> +    let securityUI = Cc[SECUREBROWSERUI_CONTRACTID].createInstance(Ci.nsISecureBrowserUI);
> +    securityUI.init(content);
> +  }

Why can't we do this directly in the script or even DOMWindowCreated?

We use "docShell" directly in the script:
 let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebProgress);

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #2)
> > +  receiveMessage: function(aMessage) {
> > +    const SECUREBROWSERUI_CONTRACTID = "@mozilla.org/secure_browser_ui;1";
> > +    let securityUI = Cc[SECUREBROWSERUI_CONTRACTID].createInstance(Ci.nsISecureBrowserUI);
> > +    securityUI.init(content);
> > +  }
> 
> Why can't we do this directly in the script or even DOMWindowCreated?

I'm not sure of the meaning of the question, but I the actual browser.xml avoid to active it depending on the disablesecurity attribute.

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#546

Maybe we could pass some parameters to the script loaded loadFrameScript directly during loading like messageManager.loadFrameScript("chrome://browser/content/content.js?disabledsecurity=true"); with "false" by default.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

Adress comments from IRC

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 451284 [details] [diff] [review]
Patch

>diff -r 0a09ff4ca657 chrome/content/bindings/browser.js

>diff -r 0a09ff4ca657 chrome/content/bindings/browser.xml

>               case "WebProgress:SecurityChange":
>                 args = [
>-                  { windowId: json.windowId, browser: this._browser, identity: json.identity },
>+                  { windowId: json.windowId, browser: this._browser, SSLStatus: json.SSLStatus },

Let's not send SSLStatus in our web progress aWebProgress parameter. Looks like our own code doesn't use it. You use:

   let currentStatus = getBrowser().securityUI.SSLStatus;

Also, it will make it harder to switch to the platform webprogress listener in the future.

>       <constructor>
>           messageManager.loadFrameScript("chrome://browser/content/bindings/browser.js", true);
>           messageManager.addMessageListener("DOMContentLoaded", this);
>           messageManager.addMessageListener("DOMTitleChanged", this);
>           messageManager.addMessageListener("DOMLinkAdded", this);
> 
>-          // Listen for first load for lazy attachment to form fill controller
>-          messageManager.addMessageListener("pageshow", this);
>-          messageManager.addMessageListener("pagehide", this);
>-          messageManager.addMessageListener("DOMPopupBlocked", this);

Why remove these?
>               case "WebProgress:SecurityChange":
>                 args = [
>-                  { windowId: json.windowId, browser: this._browser, identity: json.identity },
>+                  { windowId: json.windowId, browser: this._browser, SSLStatus: json.SSLStatus },

Same here. Do not add SSLStatus to the aWebProgress parameter

>diff -r 0a09ff4ca657 chrome/content/browser.js

>   checkIdentity: function() {

>     let state = Browser.selectedTab.getIdentityState();

Can't we use "let state = getBrowser().securityUI.state;" ? It would be consistent with status and would remove redundant code from the Tab (yes remove Tab.getIdentityState() )

>     this._lastStatus = currentStatus;
>     this._lastLocation = {};
>     try {
>       // make a copy of the passed in location to avoid cycles
>-      this._lastLocation = { host: location.host, hostname: location.hostname, port: location.port };
>+      this._lastLocation = { host: location.hostPort, hostname: location.host, port: location.port == -1 ? "" : location.port};
>     } catch (ex) { }

Let drop the try/catch - we shouldn't need it now that we are not using nsIDOMLocation

>       // Check whether this site is a security exception. XPConnect does the right
>       // thing here in terms of converting _lastLocation.port from string to int, but
>       // the overrideService doesn't like undefined ports, so make sure we have
>       // something in the default case (bug 432241).
>       if (this._overrideService.hasMatchingOverride(this._lastLocation.hostname,
>                                                     (this._lastLocation.port || 443),
>-                                                    iData.cert, {}, {}))
>+                                                    iData, {}, {}))

Can't we use iData.isException, like my patch odes? We call this same code in bindings/browser.js

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

> >       <constructor>
> >           messageManager.loadFrameScript("chrome://browser/content/bindings/browser.js", true);
> >           messageManager.addMessageListener("DOMContentLoaded", this);
> >           messageManager.addMessageListener("DOMTitleChanged", this);
> >           messageManager.addMessageListener("DOMLinkAdded", this);
> > 
> >-          // Listen for first load for lazy attachment to form fill controller
> >-          messageManager.addMessageListener("pageshow", this);
> >-          messageManager.addMessageListener("pagehide", this);
> >-          messageManager.addMessageListener("DOMPopupBlocked", this);
> 
> Why remove these?

developer error! thanks for noticing it.


> >     this._lastStatus = currentStatus;
> >     this._lastLocation = {};
> >     try {
> >       // make a copy of the passed in location to avoid cycles
> >-      this._lastLocation = { host: location.host, hostname: location.hostname, port: location.port };
> >+      this._lastLocation = { host: location.hostPort, hostname: location.host, port: location.port == -1 ? "" : location.port};
> >     } catch (ex) { }
> 
> Let drop the try/catch - we shouldn't need it now that we are not using
> nsIDOMLocation


I still see some failures on hostPort for about:home and probably on all about: pages.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 451331 [details] [diff] [review]
Patch v0.2


>diff -r 0a09ff4ca657 chrome/content/browser.js
>   checkIdentity: function() {
>-    let state = Browser.selectedTab.getIdentityState();
>-    let location = getBrowser().contentWindow.location;
>-    let currentStatus = getBrowser().securityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
>+    let state = getBrowser().securityUI.state;
>+    let location = getBrowser().currentURI;
>+    let currentStatus = getBrowser().securityUI.SSLStatus;

Add a "let browser = getBrowser();" and use the "browser" local

r+ with the nit fixed

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/c4ed0e301126