666804 - Support NetworkPrioritizer in e10s

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

NetworkPrioritizer was disabled in e10s-compat mode in bug 663042. I don't know offhand what the missing pieces are.. probably webProgress (bug 666801) and other things

Comment 1

[Drew Willcoxon :adw]

Attachment: WIP patch

The problem is that NetworkPrioritizer does this:

  getLoadgroup: function NP_BH_getLoadgroup(aBrowser) {
    return aBrowser.webNavigation.QueryInterface(Ci.nsIDocumentLoader)
                   .loadGroup.QueryInterface(Ci.nsISupportsPriority);
  },

xul:browser's remote webNavigation doesn't implement nsIDocumentLoader.

This simple patch makes the remote webNavigation implement nsISupportsPriority directly, and it remotes the nsISupportsPriority.adjustPriority() call to content.js.  I'm not sure if it's a good solution, at least partly because the add-on manager code also needs nsIDocumentLoader.loadGroup [1], but it looks like it gets an nsIDocumentLoader from a content window, not a xul:browser, so that might require a different fix altogether.

The patch isn't complete because test/browser_NetworkPrioritizer.js gets and sets nsISupportsPriority.priority, and I'm not sure how to implement that getter yet.

One thing I'm worried about is that I'm looking through a front-end keyhole at this problem, but maybe the right solution is a platform-level rethink of these interfaces.

[1] starting at http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/addonManager.js#131

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I think we should get rid of getLoadgroup entirely, and add a "priority" property on browsers that gets remoted properly (might depend on bug 662008?).

Comment 3

[:Felipe Gomes (needinfo for replies!)]

I was taking a look at where this info ends up, and although the priority/loadgroup are associated with the docloader, ultimately the information is sent back since the networking happens on the parent.
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1223

I still don't know what that means for the loadgroup/etc, but it seems it could be possible to remove the cross-process info and keep everything in the parent, if there's a way to go from the <browser> to the related http channel.

Comment 4

[Drew Willcoxon :adw]

Attachment: everything's async patch

Thanks Felipe, that's interesting.  I looked at how to access the HttpChannelParents associated with a browser, but I haven't figured out how to do it yet.  Seems like it might be possible via the browser's frame loader.

This patch kind of does what Gavin says in comment 2:  It adds browser.setPriority(), getPriority(), and adjustPriority().  getPriority is a method because it's async but needs to return a value, so it takes a callback.  I'm not saying this patch is necessarily the best solution, but it does work.

I went crazy and reworked test/browser_NetworkPrioritizer.js to use a generator to handle all the async stuff.  I have another version that uses callbacks instead if this one's too silly.  I changed the declarations of Cc, etc. in content.js because running the test caused "redeclaration of var" JS errors.  I don't understand why content.js is apparently evaluated in the same JS environment multiple times when running the test but not when running the browser.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Drew Willcoxon :adw from comment #4)
> I changed the declarations of Cc, etc. in content.js because running the test
> caused "redeclaration of var" JS errors.  I don't understand why content.js is
> apparently evaluated in the same JS environment multiple times when running the
> test but not when running the browser.

That's going to be fixed by bug 673569, I think, so probably best to avoid touching this here.

Comment 6

[Josh Matthews [:jdm]]

(In reply to Felipe Gomes (:felipe) from comment #3)
> I still don't know what that means for the loadgroup/etc, but it seems it
> could be possible to remove the cross-process info and keep everything in
> the parent, if there's a way to go from the <browser> to the related http
> channel.

I don't believe there is any association between browser elements and HTTP channels in the chrome process.

Comment 7

[Drew Willcoxon :adw]

(In reply to Josh Matthews [:jdm] from comment #6)
> I don't believe there is any association between browser elements and HTTP
> channels in the chrome process.

Right, I believe you that that's the case now, but do you know how hard it would be to create an association?  It seems wasteful to make an interprocess round trip to just to set the priority of a load group.

Comment 8

[Josh Matthews [:jdm]]

I'm probably not the best person to ask, but I believe it would be complicated. From a frameloader, you could acquire an object that QIs to nsITabParent. The C++ object for that has a method to acquire all of the PHttpChannelParent objects that are children of the PBrowserParent, and those have the actual nsHttpChannel as a private member.

Comment 9

[Drew Willcoxon :adw]

Thanks Josh.  I was looking at this chain of classes and interfaces starting at the frame loader...

(In reply to Josh Matthews [:jdm] from comment #8)
> From a frameloader, you could acquire an object that QIs to
> nsITabParent.

The TabParent* mRemoteBrowser member, right?

> The C++ object for that has a method to acquire all of the
> PHttpChannelParent objects that are children of the PBrowserParent

... but I'm not seeing this method, neither in the TabParent class declaration nor any of its base classes (the parent part of PBrowser and nsITabParent).

Comment 10

[Josh Matthews [:jdm]]

It's part of the generated IPDL code. Any PFoo that manages a sub-protocol PBar has a ManagedPBar(Parent|Child) function that returns an nsTArray of all of those actors in existence.

Comment 11

[Drew Willcoxon :adw]

Oh sweet.  (I should have read https://developer.mozilla.org/en/IPDL/Tutorial more closely.)  Thanks Josh!

I might be missing something else, but it doesn't look like PBrowser manages PHttpChannel.  It's more like:

nsFrameLoader ==(has-a)==> TabParent *mRemoteBrowser ==(is-a)==> PBrowserParent ==(is-managed-by)==> PContent ==(manages)==> PNecko ==(manages)==> PHttpChannel

Comment 12

[Josh Matthews [:jdm]]

Shoot, I'm sorry for misleading you. You are correct, there is no relationship between a TabParent and an HttpChannelParent.

Comment 13

[Benjamin Smedberg]

Yet. That's something I want to have in the future, but it depends on some changes in imagelib and perhaps elsewhere.

In any case, I don't think that doing this prioritization directly in the parent is the best decision. Can we set the priority in the *child* and associate it with the channel there?

Comment 14

[Drew Willcoxon :adw]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> Yet. That's something I want to have in the future, but it depends on some
> changes in imagelib and perhaps elsewhere.

OK, good to know.

> In any case, I don't think that doing this prioritization directly in the
> parent is the best decision.

As it stands right now, prioritization is facilitated via a load group, which lives in content and composes a set of nsIRequests, but priority adjustments (on HTTP channels, not sure about other objects) end up getting sent back to the parent.  (See Felipe's comment 3.)  So it kind of happens in both the parent and child.

> Can we set the priority in the *child* and associate it with the channel
> there?

Such that nothing is sent back to the parent?  I don't know, maybe a Necko person does?

Comment 15

[Benjamin Smedberg]

Sorry, I was replying to comment 3ff, and that we *should* (for now anyway) round-trip the priority information through the child and back to the parent, instead of trying to associate channels with a <browser> directly on the parent. So the approach of this patch is fine. Although I really don't like the getPriority/callback mechanism: can we just store the priority locally so that we don't have to go asking the content process what it was?

Comment 16

[Drew Willcoxon :adw]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> Although I really don't like the getPriority/callback
> mechanism: can we just store the priority locally so that we
> don't have to go asking the content process what it was?

That was my first idea, but I figured it's incorrect since something external to the browser could change the load group's priority, like a content script in some extension (or God forbid some other part of the front-end and we forget to coordinate it), and there doesn't seem to be a way to listen for priority changes.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Maybe we should add one? [add|remove]PriorityObserver(in nsIObserver)?

Comment 18

[Drew Willcoxon :adw]

In this case the observer would live in the parent -- and in JS -- and the observee would live in the child.  All IPC AFAIK is facilitied via IPDL, so there would need to be a PObserver protocol that the observer in JS could hook into somehow or implement, which I don't think is possible.  So it would be more like a general scriptable and IPC'able observer component written in C++ and instantiated and customized in the JS.

Maybe such a general purpose IPC'able observer component would be useful in a bunch of places, but if not that seems like a worse solution.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

I meant an observer that lived entirely in the child, and notified the parent to update its cache of the priority when it changed.

Comment 20

[Drew Willcoxon :adw]

Same difference?

Comment 21

[Drew Willcoxon :adw]

Sorry, I'm dumb.  An observer in the child and then message passing up to the parent, I see.

Comment 22

[Drew Willcoxon :adw]

Attachment: observer patch

Comment 23

[Drew Willcoxon :adw]

Attachment: better observer patch

This isolates the sequence number stuff in content.js instead of leaking it into browser.

Comment 24

[Drew Willcoxon :adw]

Attachment: observer patch 3

Another simplification that takes advantage of the fact that the priority setter and observer notification happen synchronously in the child.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 566614 [details] [diff] [review]
observer patch 3

I think that rather than adding a generic nsISupportsPriorityObservee, you could just add these methods to nsILoadGroup?

>diff --git a/browser/base/content/content.js b/browser/base/content/content.js

>-const Cc = Components.classes;
>-const Ci = Components.interfaces;
>-const Cu = Components.utils;
>+let Cc = typeof(Cc) == "undefined" ? Components.classes : Cc;
>+let Ci = typeof(Ci) == "undefined" ? Components.interfaces : Ci;
>+let Cu = typeof(Cu) == "undefined" ? Components.utils : Cu;

I think bug 673569 will be fixing this in a more general way, probably best not to change this here.

>+let ignorePriorityAdjusted = false;
>+
>+addMessageListener("LoadGroup:setPriority",

Browser:LoadGroup:setPriority? I guess there is no existing consistency here :( Namespacing <browser>-related messages together seems like a good idea, but I don't know how feasible it is to rename the existing ones.

>+      sendAsyncMessage("LoadGroup:priorityAdjusted", {

It might actually help in this case - this message should be a <browser> implementation detail, not something that people come to rely on generically for observing loadgroup changes.

>diff --git a/toolkit/content/widgets/browser.xml b/toolkit/content/widgets/browser.xml

>+      <field name="_priority">null</field>

You could just set PRIORITY_NORMAL as the default value here and avoid the null check in the getter, right?

>+      <property name="priority">

>+        <setter>
>+          <![CDATA[
>+            let priority = Number(val);
>+            if (isFinite(priority)) {

It's probably more useful to just throw if isNaN(parseInt(val)).

Comment 26

[Drew Willcoxon :adw]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #25)
> I think that rather than adding a generic nsISupportsPriorityObservee, you
> could just add these methods to nsILoadGroup?

The priority-related properties are declared in nsISupportsPriority, not nsILoadGroup.

But did you see bug 691614 comment 3?  It seems like a lot of this e10s work is going to involve sending messages to docShell, or some property that hangs off docShell.  We're not going to add observers for every one of those properties, right?  So maybe we should forget about it here.

> Browser:LoadGroup:setPriority? I guess there is no existing consistency here
> :( Namespacing <browser>-related messages together seems like a good idea,
> but I don't know how feasible it is to rename the existing ones.

Oh, so if some other unrelated messageManager happens to use the same message name, it would conflict in some way?  I would think that calling window.messageManager.loadFrameScript("content.js") in browser.js would mean that content.js only receives messages sent via that messageManager, and not messages sent by a different messageManager in some other part of the front end.

What's the "scope" of messages?  If two different messageManagers load different frame scripts, do both scripts receive messages sent by one or the other messageManager?  What if they load the same frame script?

Comment 27

[:Gavin Sharp [email: [e-mail]]]

(In reply to Drew Willcoxon :adw from comment #26)
> But did you see bug 691614 comment 3?  It seems like a lot of this e10s work
> is going to involve sending messages to docShell, or some property that
> hangs off docShell.  We're not going to add observers for every one of those
> properties, right?

You're right, this could get out of hand. Can we have the underlying object register with its associated messageManager directly, rather than going through a second intermediate layer in the frame script? That is, call addMessageListener("LoadGroup:setPriority") on the proper messageManager from the actual underlying nsILoadGroup implementation? That would remove the need for the observer indirection in the child.

> Oh, so if some other unrelated messageManager happens to use the same
> message name, it would conflict in some way?

I'm not worried about other message managers, I'm worried about other consumers in the parent process noticing that there's a "LoadGroup:priorityAdjusted" message, and using it to track the loadgroup state, when that message is really specifically intended for the <browser> implementation to update its cache (e.g. it won't be fired for <browser>-triggered changes).

Comment 28

[Drew Willcoxon :adw]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #27)
> That is, call addMessageListener("LoadGroup:setPriority") on
> the proper messageManager from the actual underlying
> nsILoadGroup implementation? That would remove the need for the
> observer indirection in the child.

I guess so, but that's not much better than going through the content.js middleman.  It's not the middleman that I don't like but the thought of having to implement listeners for -- or modify the implementations of -- all these different native properties.

Just thinking out loud.  Apple's Cocoa APIs have this generic notion of "key-value observing": for any object that supports it, you can register an observer with it that will be notified when any property reachable from it changes.  Like JavaScript proxy catch-alls but a little more powerful.  So here for example, maybe webNavigation.loadGroup.addKeyValueObserver("priority", function () {}).  In bug 691614, docShell.contentViewer.addKeyValueObserver("textZoom").  An observer framework like that could probably be implemented once at the XPConnect level.  But I guess that would only catch changes made from JS and not native code.

(In reply to Gavin Sharp (use [e-mail] for email) from comment #27)
> I'm not worried about other message managers, I'm worried about other
> consumers in the parent process

Couldn't we just tell add-on authors that this stuff is private?  Just like we could tell them to go through the browser rather than modifying docshell properties via content scripts.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

(In reply to Drew Willcoxon :adw from comment #28)
> I guess so, but that's not much better than going through the content.js
> middleman.  It's not the middleman that I don't like but the thought of
> having to implement listeners for -- or modify the implementations of -- all
> these different native properties.

I agree re: "implementing listeners for", but "modify the implementations of" doesn't seem like it would be too bad. We shouldn't be too averse to changing underlying implementations - shimming to minimize changes now might seem attractive, but the maintenance costs of the added complexity can really hit you over time.

> Couldn't we just tell add-on authors that this stuff is private?

I wasn't just thinking about addons, I was thinking about us writing code in the future. But it was just naming nit based on a theoretical concern, it's not a big deal either way :)

Comment 30

[Drew Willcoxon :adw]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #27)
> That is, call addMessageListener("LoadGroup:setPriority") on
> the proper messageManager from the actual underlying
> nsILoadGroup implementation? That would remove the need for the
> observer indirection in the child.

Thought about this a little more and found another reason I'm opposed to it, which is that it means tying a component way down in netwerk to browser windows, right?

Comment 31

[:Gavin Sharp [email: [e-mail]]]

The messages managers aren't browser-specific and don't depend on windows. The tricky part here is that the nsLoadGroup doesn't have any references to its parent (and thus can't easily access the relevant message manager). But it looks to me like the nsDocLoader should be able to somehow reference its owning TabChild, which can in turn access its messagemanager, so I think my crazy plan might work in theory (the object hierarchy is a bit confusing and I'm not sure that all makes sense). If we only expose adjustPriority() on the browser we don't even need to worry about keeping a cache of the priority value up to date if somehow someone other than the docloader adjusts the loadgroup's priority.

Comment 32

[Drew Willcoxon :adw]

Attachment: simple adjustPriority patch

(In reply to Gavin Sharp (use [e-mail] for email) from comment #31)
> If we only expose adjustPriority() on the browser we don't even
> need to worry about keeping a cache of the priority value up to
> date if somehow someone other than the docloader adjusts the
> loadgroup's priority.

This does that.  If it's OK then I'll update the test and ask for review.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 570811 [details] [diff] [review]
simple adjustPriority patch

Seems reasonable, but we probably don't want the generic <browser> API to depend on browser's content.js script, so the addMessageListener call should probably go in a new content script loaded by the browser binding. Makes me wonder about the performance implications of adding many frame scripts...

Comment 34

[Drew Willcoxon :adw]

Attachment: patch

Comment 35

[:Gavin Sharp [email: [e-mail]]]

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

Cancelling e10s review request since we're not going to be pursuing e10s on desktop.

Comment 36

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: network-prioritizer

This was actually pretty easy using the foundation we have in place now.

Comment 37

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: network-prioritizer v2

Oops, forgot qref.

Comment 38

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b90ce080a83

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b90ce080a83