If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Pull identity data from message data

RESOLVED FIXED

Status

Fennec Graveyard
General
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mfinkle, Assigned: vingtetun)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 451035 [details] [diff] [review]
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.
Attachment #451035 - Flags: review?(21)
Created attachment 451155 [details] [diff] [review]
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
> +  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);
(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.
Created attachment 451284 [details] [diff] [review]
Patch

Adress comments from IRC
Attachment #451035 - Attachment is obsolete: true
Attachment #451155 - Attachment is obsolete: true
Attachment #451284 - Flags: review?(mark.finkle)
Attachment #451035 - Flags: review?(21)
(Reporter)

Updated

7 years ago
Attachment #451284 - Flags: review?(mark.finkle) → review-
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
Created attachment 451331 [details] [diff] [review]
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.
Attachment #451284 - Attachment is obsolete: true
Attachment #451331 - Flags: review?(mark.finkle)
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
Attachment #451331 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/c4ed0e301126
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

6 years ago
Assignee: nobody → 21
You need to log in before you can comment on or make changes to this bug.