Closed Bug 666804 Opened 9 years ago Closed 6 years ago

Support NetworkPrioritizer in e10s

Categories

(Firefox :: Tabbed Browser, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: Felipe, Assigned: billm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [e10s])

Attachments

(1 file, 8 obsolete files)

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
Depends on: 663042
Blocks: fxe10s
Assignee: nobody → adw
Attached patch WIP patch (obsolete) — Splinter Review
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
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?).
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.
Attached patch everything's async patch (obsolete) — Splinter Review
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.
Attachment #563236 - Attachment is obsolete: true
Attachment #564978 - Flags: review?(gavin.sharp)
(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.
(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.
(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.
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.
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).
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.
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
Shoot, I'm sorry for misleading you. You are correct, there is no relationship between a TabParent and an HttpChannelParent.
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?
(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?
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?
(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.
Maybe we should add one? [add|remove]PriorityObserver(in nsIObserver)?
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.
Status: NEW → ASSIGNED
I meant an observer that lived entirely in the child, and notified the parent to update its cache of the priority when it changed.
Same difference?
Sorry, I'm dumb.  An observer in the child and then message passing up to the parent, I see.
Attached patch observer patch (obsolete) — Splinter Review
Attachment #566577 - Flags: review?(gavin.sharp)
Attached patch better observer patch (obsolete) — Splinter Review
This isolates the sequence number stuff in content.js instead of leaking it into browser.
Attachment #566577 - Attachment is obsolete: true
Attachment #566586 - Flags: review?(gavin.sharp)
Attachment #566577 - Flags: review?(gavin.sharp)
Attached patch observer patch 3 (obsolete) — Splinter Review
Another simplification that takes advantage of the fact that the priority setter and observer notification happen synchronously in the child.
Attachment #566586 - Attachment is obsolete: true
Attachment #566614 - Flags: review?(gavin.sharp)
Attachment #566586 - Flags: review?(gavin.sharp)
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)).
Attachment #566614 - Flags: review?(gavin.sharp) → feedback+
Attachment #564978 - Flags: review?(gavin.sharp)
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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?
(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).
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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 gavin@gavinsharp.com 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.
(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 :)
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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?
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.
Attached patch simple adjustPriority patch (obsolete) — Splinter Review
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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.
Attachment #564978 - Attachment is obsolete: true
Attachment #566614 - Attachment is obsolete: true
Attachment #570811 - Flags: feedback?(gavin.sharp)
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...
Attachment #570811 - Flags: feedback?(gavin.sharp) → feedback+
Attached patch patch (obsolete) — Splinter Review
Attachment #570811 - Attachment is obsolete: true
Attachment #571175 - Flags: review?(gavin.sharp)
Comment on attachment 571175 [details] [diff] [review]
patch

Cancelling e10s review request since we're not going to be pursuing e10s on desktop.
Attachment #571175 - Flags: review?(gavin.sharp)
Assignee: adw → nobody
Status: ASSIGNED → NEW
Attached patch network-prioritizer (obsolete) — Splinter Review
This was actually pretty easy using the foundation we have in place now.
Assignee: nobody → wmccloskey
Attachment #571175 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8387879 - Flags: review?(felipc)
Oops, forgot qref.
Attachment #8387879 - Attachment is obsolete: true
Attachment #8387879 - Flags: review?(felipc)
Attachment #8387888 - Flags: review?(felipc)
Attachment #8387888 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/9b90ce080a83
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.