588441 - Remove progress listeners in chrome

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: WIP

This adds extra messages in content so that chrome does not have to add a progress listener, which significantly reduces the messages being passed during load.

I tested this with two phones side by side and didn't see any difference in page load times.  However, it may be more responsive during load than before.  We need more testing for this.

Comment 1

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

Comment on attachment 467038 [details] [diff] [review]
WIP

I don't think I like this. Too specific to Fennec's use case. The bindings/browser.js stuff will live in toolkit and I don't like putting specific stuff for Fennec.

Comment 2

[Benjamin Stover (:stechz)]

Really?  Network start and network stop are both things that extensions would care about if they are using progress listeners.

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 467038 [details] [diff] [review]
WIP

Seems reasonable to me; I like keeping more of this contained in the child process.  I have no opinion on the question above (whether this is suitable for toolkit).

Comment 4

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

Currently, the patch has the Browser receiving the messages. I think this should be moved to the Tab. I don't like dumping too much stuff in Browser, especially when it's tab-specific.

I definitely do not want to break the progress listener approach. This patch simply swaps the nsIWebProgessListener interface for listening for messages directly. But we should not break the nsIWebProgressListener approach for other code.

Lastly, I agree NetworkStart, NetworkStart and DocumentStop are handy, I'm not sure I have seen too many add-ons listening specifically for those states.

If we don't break nsIWebProgressListener and we see a pageload improvement, I could be swayed. Just remember, content.js and others are Fennec specific, but bindings/browser.js is _not_ and should not become Fennec specific.

Comment 5

[Benjamin Stover (:stechz)]

> Lastly, I agree NetworkStart, NetworkStart and DocumentStop are handy, I'm not
> sure I have seen too many add-ons listening specifically for those states.

What exactly *are* extensions using progress listeners for?  I'm personally fine with breaking them because they are changing anyway.  The progress listener kinda-sorta looks the same, but don't look too close or the magic blows up in your face.

Message listeners are a great solution for this.  It moves progress listeners in parent process to a new expectation: that everything they receive is just a JSON-deserialized object.  And we get the awesome side benefit that developers don't have to do all this QI crap, or worry about strong vs weak references, or need to have a ton of dummy functions, or do anything with nsIWebProgressListener.flags.

If we want to make extension developers lives easier from now on, this is it.  Make them never have to touch a progress listener again, and they'll be happy to get rid of it.

Comment 6

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

Attachment: patch

This patch is similar to Ben's but with some minor refactoring differences. In general, the patch:
* Removes the browser.webProgress property and ability to add progress listeners to a browser (more below for rationale)
* Changes to use messages directly, not nsIWebProgressListener in browser front-end code
* Removes the unused progress listener in content.js
* Limits the progress listener messages to only the top level window. This limits the message traffic. (more below for rationale)
* Limits the listeners and messages to state, location and security - no progress or status callbacks or messages are created. This limits CPU usage during page load (more below for rationale)
* Creates a singleton listener in the front-end code - kinda like tabbrowser in desktop firefox. Just limits the message listeners active in the application, but not the actual message traffic.

Rationale: I once wanted to maintain the browser.webProgress mechanism, mainly for add-on developers, but I have changed my mind over time. The cost is too high (message traffic and CPU usage). The code is too complex (a single webprogress listener in the browser widget can not handle all variations efficiently or elegantly).

If an add-on wants to listen for web progress events, it should add a progress listener in a frame script and use messages as needed. That is the philosophy we have taken for everything else in multi-process Fennec. This is no different.

Also, if an add-on can use our minimal "Content:Xxx" web progress messages, great! But, just as our DOM messages, they are for internal use only and are subject to change.

Comment 7

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

Note: I changed from "WebProgress:Xxx" -> "Content:Xxx" for the message names because these are now specific to certain code, not general purpose web progress listeners messages.

Comment 8

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

Oops, I left some debugging dumps in the patch that I already removed locally.

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 502513 [details] [diff] [review]
patch

>+++ b/chrome/content/bindings/browser.js

>   onStateChange: function onStateChange(aWebProgress, aRequest, aStateFlags, aStatus) {
>+    if (content != aWebProgress.DOMWindow)
>+      return;
>+
>     let webProgress = Ci.nsIWebProgressListener;
>     let notifyFlags = 0;
>     if (aStateFlags & webProgress.STATE_IS_REQUEST)
>       notifyFlags |= Ci.nsIWebProgress.NOTIFY_STATE_REQUEST;
>     if (aStateFlags & webProgress.STATE_IS_DOCUMENT)
>       notifyFlags |= Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT;
>     if (aStateFlags & webProgress.STATE_IS_NETWORK)
>       notifyFlags |= Ci.nsIWebProgress.NOTIFY_STATE_NETWORK;
>     if (aStateFlags & webProgress.STATE_IS_WINDOW)
>       notifyFlags |= Ci.nsIWebProgress.NOTIFY_STATE_WINDOW;

I think we can completely remove the notifyFlags variable, along with the _notifyFlags and _calculatedNotifyFlags properties.

>+++ b/chrome/content/bindings/browser.xml

>+              case "Content:SecurityChange":
>+                let serhelper = Cc["@mozilla.org/network/serialization-helper;1"].getService(Ci.nsISerializationHelper);
>+                let SSLStatus = json.SSLStatusAsString ? serhelper.deserializeObject(json.SSLStatusAsString) : null;
>+                if (SSLStatus) {
>+                  SSLStatus.QueryInterface(Ci.nsISSLStatus);
>+                  if (SSLStatus) {
>+                    // We must check the Extended Validation (EV) state here, on the chrome
>+                    // process, because NSS is needed for that determination.
>+                    let ii = SSLStatus.serverCert.QueryInterface(Ci.nsIIdentityInfo);
>+                    if (ii && ii.isExtendedValidation)
>+                      json.state |= Ci.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL;
>+                  }
>+                }
> 
>+                let data = this._getIdentityData(SSLStatus);
>                 this._browser._securityUI = {
>+                  SSLStatus: SSLStatus ? {serverCert: data} : null,
>                   state: json.state
>                 }
>                 this._browser.updateWindowId(json.contentWindowId);
>                 break;
>             }
>           },
> 
>+          /**
>+           * Helper to parse out the important parts of the SSL cert for use in constructing
>+           * identity UI strings
>+           */
>+          _getIdentityData: function(status) {
>+            let result = {};
>+
>+            if (status) {
>+              let cert = status.serverCert;
>+
>+              // Human readable name of Subject
>+              result.subjectOrg = cert.organization;
>+
>+              // SubjectName fields, broken up for individual access
>+              if (cert.subjectName) {
>+                result.subjectNameFields = {};
>+                cert.subjectName.split(",").forEach(function(v) {
>+                  var field = v.split("=");
>+                  if (field[1])
>+                    this[field[0]] = field[1];
>+                }, result.subjectNameFields);
>+
>+                // Call out city, state, and country specifically
>+                result.city = result.subjectNameFields.L;
>+                result.state = result.subjectNameFields.ST;
>+                result.country = result.subjectNameFields.C;
>               }
>+
>+              // Human readable name of Certificate Authority
>+              result.caOrg =  cert.issuerOrganization || cert.issuerCommonName;
>+
>+              if (!this._overrideService)
>+                this._overrideService = Cc["@mozilla.org/security/certoverride;1"].getService(Ci.nsICertOverrideService);
>+
>+              // Check whether this site is a security exception.
>+              let currentURI = this._browser._webNavigation._currentURI;
>+              result.isException = !!this._overrideService.hasMatchingOverride(currentURI.asciiHost, currentURI.port, cert, {}, {});
>+            }
>+
>+            return result;
>           }

I really don't like that all this code is duplicated in the local and remote bindings.  Can we move all this stuff out of the _webProgress object and into methods on the binding, so it can be re-used or overridden as needed?  (Or find some other way to share it...)

>+++ b/chrome/content/browser.js

>+      case "Content:LocationChange": {
>+        let spec = json.location;
>+        let location = spec.split("#")[0]; // Ignore fragment identifier changes.

This isn't necessary; "location" already has the fragment removed in browser.xml.

>+        tab.hostChanged = true;

Not caused by this patch, but: This variable isn't really accurately named, and I think it should be set inside the "if (locationHasChanged)" block below.

Comment 10

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

Attachment: patch 2

Removes notifyFlags
Removes duplicate webProgressListener
Moves tab.hostChanged

json.location is not split the way you think. "location" is split in bindings/browser.js - but that is not sent with the message.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 502570 [details] [diff] [review]
patch 2

You should also remove the _notifyFlags and _calculatedNotifyFlags properties from the global WebProgressListener object at the top of bindings/browser.js.

Comment 12

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

Attachment: patch 2 - unbitrotted

Just unbitrotted

Comment 13

[Benjamin Stover (:stechz)]

Comment on attachment 502623 [details] [diff] [review]
patch 2 - unbitrotted

Just curious. Do we still use window ID anywhere? It might still be useful, but if it's not we could also get rid of that property.

In chrome/content/browser.js:
>+  this.hostChanged = false;
>+  this.state = null;
>+

Since host changed is tracked in content process, do we need this hostChanged now?

Comment 14

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

(In reply to comment #13)
> Comment on attachment 502623 [details] [diff] [review]
> patch 2 - unbitrotted
> 
> Just curious. Do we still use window ID anywhere? It might still be useful, but
> if it's not we could also get rid of that property.

We pass it for some DOMContentLoaded and pageshow messages. It's still the only way we have to track DOMWindows / Tabs across the process. Some add-ons have started using it and the new nsIScriptError2 interface uses it.

I'm in no hurry to remove it.

> In chrome/content/browser.js:
> >+  this.hostChanged = false;
> >+  this.state = null;
> 
> Since host changed is tracked in content process, do we need this hostChanged
> now?

We probably could, but I want to test that assumption a bit more. Filing a followup bug.

Comment 15

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

pushed:
http://hg.mozilla.org/mobile-browser/rev/3e12e0b7c305

Small Ts and Txul improvements. I hope page load is better too.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: followup

Missed one contentObject->Content change in fullscreen-video.js.  Thanks to Fabrice for catching this.

This patch also changes the synthesized event order to mousemove,mousedown,mouseup, to match our synthesized clicks in content.js.

Comment 17

[Matt Brubeck (:mbrubeck)]

Pushed followup: http://hg.mozilla.org/mobile-browser/rev/128e3976d7f6