1244227 - add back end network speed throttling

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Tom Tromey :tromey]

Add back end support for the network throttling control project.
See bug 1110207 for the user story, bug 1239925 for the breakdown bug.

Comment 1

[Tom Tromey :tromey]

Attachment: P

Preliminary patch.

This throttles ok but has a few issues:

* Obviously I didn't wire it up to network-monitor in a good way.
  There should really be just a single manager per window.
  Not a big problem.

* There doesn't seem to be a way to intercept uploads.
  I'm still digging around in platform to see where this might be done.

* The network panel timings are all off when using this code.
  This happens because the timings are based on the http-on-* events
  (see nsIHttpProtocolHandler.idl); but these are emitted by the
  platform, so aren't affected by our throttling.
  I'm not totally sure how I'm going to handle this yet; but one idea
  might be to route these events through the NetworkThrottleManager.

Comment 2

[Tom Tromey :tromey]

I remembered one other possible issue -- we might reasonably want to slow down DNS
requests as well.  I haven't investigated this yet at all.

Comment 3

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #1)

> * There doesn't seem to be a way to intercept uploads.
>   I'm still digging around in platform to see where this might be done.

My current idea is to add a new method to nsITraceableChannel, say
|setNewUploadListener|, then update nsHttpChannel to use this channel,
when set, when calling mTransactionPump->AsyncRead.

network-monitor can call the new method from an http-on-modify-request
event handler.

Comment 4

[Tom Tromey :tromey]

FWIW I tend to think the "offline" case should be implemented differently.
Bug 1156659 could be used for this.

Comment 5

[Tom Tromey :tromey]

Attachment: nsITraceableChannel.setNewUploadListener

Platform support for upload throttling.

Comment 6

[Tom Tromey :tromey]

Attachment: add network throttling for downloads

Update throttler to handle uploads as well.

Comment 7

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #2)
> I remembered one other possible issue -- we might reasonably want to slow
> down DNS
> requests as well.  I haven't investigated this yet at all.

One possibility here is to add yet another method to nsITraceableChannel,
e.g., "setDNSListener".  This could be called from the http-on-modify-request
handler.

However, the simplest way to do this would involve just inserting a new
delaying nsIDNSListener.  But, that wouldn't affect the reported timings,
as nsHttpChannel gets those from the nsDNSPrefetch object.

Comment 8

[Tom Tromey :tromey]

FWIW now I think that upload interception patch is just completely wrong.

Comment 9

[Tom Tromey :tromey]

Attachment: make network-monitor eslint-clean

MozReview-Commit-ID: AnaXFFHlNLM

Comment 10

[Tom Tromey :tromey]

Attachment: remove obsolete TODO from network-monitor.js

MozReview-Commit-ID: Lbx8C41oSqG

Comment 11

[Tom Tromey :tromey]

Attachment: add nsIHttpChannel.setUploadThrottleRate

MozReview-Commit-ID: JVIjxEO901W

This is a new approach to throttling the request.

Comment 12

[Tom Tromey :tromey]

Attachment: NetworkResponseListener does not need to implement nsIInterfaceRequestor

MozReview-Commit-ID: 9TMlmdjQWLL

Comment 13

[Tom Tromey :tromey]

Attachment: add network throttling

MozReview-Commit-ID: Iy6buFxUrGg

Comment 14

[Tom Tromey :tromey]

Attachment: properly report throttled network timing

MozReview-Commit-ID: BCJLSRGS0vE

Comment 15

[Tom Tromey :tromey]

This seems to work in my local testing.

What remains now is test cases, and adding a reasonable way for the client to request
throttling, as opposed to hard-coding it in NetworkMonitor.

Comment 16

[Tom Tromey :tromey]

Comment on attachment 8722689 [details] [diff] [review]
add nsIHttpChannel.setUploadThrottleRate

I'd like to get feedback on whether this is a reasonable way to implement
upload throttling; or alternatively if there is some other approach I
should be taking.

The patch is incomplete currently -- it does not have documentation for
the new method, and it doesn't have a test case.

Comment 17

[Patrick McManus [:mcmanus]]

Comment on attachment 8722689 [details] [diff] [review]
add nsIHttpChannel.setUploadThrottleRate

Review of attachment 8722689 [details] [diff] [review]:
-----------------------------------------------------------------

broadly, I think this is fine.. I just read it enough to get the gist - not in detail. the biggest question I have is probably how the stream consumer knows when it can read more after it has exceeded the throttle quota.. I would have expected it to be nsIAsyncInputStream

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +307,5 @@
>      // Returns an https URI for channels that need to go through secure
>      // upgrades.
>      static nsresult GetSecureUpgradedURI(nsIURI* aURI, nsIURI** aUpgradedURI);
>  
> +    already_AddRefed<nsIInputStream> ThrottleUploadStream(nsIInputStream* aStream);

I don't think this should be a channel method.. it could probably be a static method on ThrottleInputStream as long as you can have accessors for the rates on the channel.

::: netwerk/protocol/http/ThrottleInputStream.cpp
@@ +37,5 @@
> +  return mStream->Available(aResult);
> +}
> +
> +uint32_t
> +ThrottleInputStream::Throttle(uint32_t aCount)

I haven't closely read the logic here, but we already have netwerk/base/EventTokenBucket - not sure if it would work for you or not

@@ +39,5 @@
> +
> +uint32_t
> +ThrottleInputStream::Throttle(uint32_t aCount)
> +{
> +  PRTime now = PR_Now();

mozilla::Timestamp rather than PRTime

::: netwerk/protocol/http/ThrottleInputStream.h
@@ +43,5 @@
> +  };
> +
> +  nsCOMPtr<nsIInputStream> mStream;
> +  nsTArray<ThrottleEntry> mReadEvents;
> +  double mMeanBytesPerSecond;

is there a reason uint32/64 can't do these? I really have an aversion to floating point.

::: netwerk/protocol/http/nsIHttpChannel.idl
@@ +397,5 @@
>       *         started to deliver the content to its listener.
>       */
>      void redirectTo(in nsIURI aTargetURI);
>  
> +    void setUploadThrottleRate(in double aMeanBytesPerSecond, in double aMaxBytesPerSecond);

rather than making this a method of nsIhttpChannel.idl, it makes more sense to me that its a pair of attributes on a new interface (nsIThrottledInputChannel or something) that httpchannel can implement.. also make sure that e10s can set them content

Comment 18

[Tom Tromey :tromey]

(In reply to Patrick McManus [:mcmanus] from comment #17)
> Comment on attachment 8722689 [details] [diff] [review]
> add nsIHttpChannel.setUploadThrottleRate
> 
> Review of attachment 8722689 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> broadly, I think this is fine.. I just read it enough to get the gist - not
> in detail. the biggest question I have is probably how the stream consumer
> knows when it can read more after it has exceeded the throttle quota.. I
> would have expected it to be nsIAsyncInputStream

I found this a bit weird as well, but I think the request stream is polled
by something else (that I found once before but now cannot).
The current mRequestStream is also not async AFAICT, see:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#372

Comment 19

[Patrick McManus [:mcmanus]]

nothing is really poll driven that I know of but obviously its usually the other side of the pipe in practice that has backed up (i.e. sending the stuff out to the network) so perhaps it has gone by as a moot point but would be a problem with this patch :(

Comment 20

[Tom Tromey :tromey]

(In reply to Patrick McManus [:mcmanus] from comment #19)
> nothing is really poll driven that I know of but obviously its usually the
> other side of the pipe in practice that has backed up (i.e. sending the
> stuff out to the network) so perhaps it has gone by as a moot point but
> would be a problem with this patch :(

I opened the debugger again and took a look.
What I'm thinking of specifically is here:

(gdb) bt
#0  mozilla::net::nsHttpTransaction::ReadSegments (this=0x7f405bee3800, reader=0x7f404fa43aa0, count=32768, countRead=0x7f407a3ac958) at /home/tromey/firefox-git/hg2/netwerk/protocol/http/nsHttpTransaction.cpp:702
#1  0x00007f4087471576 in mozilla::net::nsAHttpTransaction::ReadSegmentsAgain (this=0x7f405bee3800, reader=0x7f404fa43aa0, count=32768, countRead=0x7f407a3ac958, again=0x7f407a3ac957) at /home/tromey/firefox-git/hg2/netwerk/protocol/http/nsAHttpTransaction.h:85
#2  0x00007f408751c241 in mozilla::net::nsHttpConnection::OnSocketWritable (this=0x7f404fa43aa0) at /home/tromey/firefox-git/hg2/netwerk/protocol/http/nsHttpConnection.cpp:1599
#3  0x00007f408751dbc5 in mozilla::net::nsHttpConnection::OnOutputStreamReady (this=0x7f404fa43aa0, out=0x7f405bee3648) at /home/tromey/firefox-git/hg2/netwerk/protocol/http/nsHttpConnection.cpp:2105
#4  0x00007f4087335108 in nsSocketOutputStream::OnSocketReady (this=0x7f405bee3648, condition=nsresult::NS_OK) at /home/tromey/firefox-git/hg2/netwerk/base/nsSocketTransport2.cpp:552
#5  0x00007f408733991b in nsSocketTransport::OnSocketReady (this=0x7f405bee3400, fd=0x7f4050ce1a00, outFlags=2) at /home/tromey/firefox-git/hg2/netwerk/base/nsSocketTransport2.cpp:1931
#6  0x00007f408733ff47 in nsSocketTransportService::DoPollIteration (this=0x7f409b1a7800, pollDuration=0x7f407a3acb20) at /home/tromey/firefox-git/hg2/netwerk/base/nsSocketTransportService2.cpp:1099
[...]


I think what happens is that nsHttpTransaction is called whenever the socket is writable,
but the throttling only lets some number of bytes out each time.  So, it just tries again
until it succeeds.  Now, I thought at some point I found a rate-limiter in here somewhere,
but I can't find it now and I assume I was just misreading the code.

I suppose this is pretty inefficient.

Comment 21

[Tom Tromey :tromey]

Attachment: make network-monitor eslint-clean

MozReview-Commit-ID: AnaXFFHlNLM

Comment 22

[Tom Tromey :tromey]

Attachment: remove obsolete TODO from network-monitor.js

MozReview-Commit-ID: Lbx8C41oSqG

Comment 23

[Tom Tromey :tromey]

Attachment: add nsIThrottledInputChannel.idl and implement

MozReview-Commit-ID: JVIjxEO901W

Comment 24

[Tom Tromey :tromey]

Attachment: NetworkResponseListener does not need to implement nsIInterfaceRequestor

MozReview-Commit-ID: 9TMlmdjQWLL

Comment 25

[Tom Tromey :tromey]

Attachment: add network throttling

MozReview-Commit-ID: Iy6buFxUrGg

Comment 26

[Tom Tromey :tromey]

Attachment: properly report throttled network timing

MozReview-Commit-ID: BCJLSRGS0vE

Comment 27

[Tom Tromey :tromey]

Attachment: add an API to enable throttling

MozReview-Commit-ID: BirjFHVSZN7

Comment 28

[Tom Tromey :tromey]

Comment on attachment 8726776 [details] [diff] [review]
make network-monitor eslint-clean

This patch is a reasonably straightforward eslint cleanup of network-monitor.js.

Comment 29

[Tom Tromey :tromey]

Comment on attachment 8726777 [details] [diff] [review]
remove obsolete TODO from network-monitor.js

This is trivia -- while doing the eslint cleanup, I noticed an obsolete comment.
I separated this out so I'd remember to point it out for review.

Comment 30

[Tom Tromey :tromey]

Comment on attachment 8726778 [details] [diff] [review]
add nsIThrottledInputChannel.idl and implement

After a few rounds of confusion I arrived at this patch.

I finally realized my error with polling.  If the input stream
would block, we hit this code, which tries to read async:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpTransaction.cpp#721

So, ThrottleInputStream now also implements nsIAsyncInputStream.

Also, I needed to share a single throttler among several streams,
and this affected the design.

I did look at EventTokenBucket but it didn't seem to fit my needs;
and in the end the timer code is quite small.

Comment 31

[Tom Tromey :tromey]

Attachment: make network-monitor eslint-clean

After rebasing I noticed one new eslint warning.

MozReview-Commit-ID: AnaXFFHlNLM

Comment 32

[Tom Tromey :tromey]

Comment on attachment 8726848 [details] [diff] [review]
make network-monitor eslint-clean

This patch is a reasonably straightforward eslint cleanup of network-monitor.js.

Comment 33

[Tom Tromey :tromey]

Attachment: NetworkResponseListener does not need to implement nsIInterfaceRequestor

Minor cleanup to previous version of this patch.

MozReview-Commit-ID: 9TMlmdjQWLL

Comment 34

[Tom Tromey :tromey]

Comment on attachment 8726850 [details] [diff] [review]
NetworkResponseListener does not need to implement nsIInterfaceRequestor

The use of nsIInterfaceRequestor in network-monitor caused me a lot of confusion.
It means that the NetworkResponseListener "reaches around" any other listeners
to listen to some other set of events on the channel.
I couldn't see any reason this is really needed, so this patch removes it.

Comment 35

[Tom Tromey :tromey]

Comment on attachment 8726780 [details] [diff] [review]
add network throttling

This adds the basics of the network throttler.

The download throttling and the throttling manager are in a new file.
network-monitor is changed to handle this -- but this leaves actual
instantiation of a throttler to a later patch.

This patch contains a unit test for the new code, but no integration
tests.  Those must also wait until there's a way to instantiate a
throttler.

Comment 36

[Tom Tromey :tromey]

Comment on attachment 8726781 [details] [diff] [review]
properly report throttled network timing

The previous patch throttled network data -- but, strangely, did not affect
the reported timings.  This happens because timings are collected from an
entirely different interface, which bypasses the throttled listeners.

This patch changes http activity event reporting such that, if throttling
is in place, the relevant events will be routed through the throttled
listener and then reported when appropriate.

Comment 37

[Tom Tromey :tromey]

Comment on attachment 8726782 [details] [diff] [review]
add an API to enable throttling

Now we can finally add an API to let the client instantiate a network throttler.
This also includes the integration test.

Comment 38

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8726850 [details] [diff] [review]
NetworkResponseListener does not need to implement nsIInterfaceRequestor

Review of attachment 8726850 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/webconsole/network-monitor.js
@@ +384,4 @@
>                                 available);
>          }
>        }
> +      this.transferredSize += available;

Will this size be the "on the wire" size in the case of gzip encoding, etc.?  That's what the previous code was after.

Comment 39

[Tom Tromey :tromey]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38)

> Will this size be the "on the wire" size in the case of gzip encoding, etc.?
> That's what the previous code was after.

I will double check to make sure.

Comment 40

[Tom Tromey :tromey]

Comment on attachment 8726848 [details] [diff] [review]
make network-monitor eslint-clean

I'm moving this to bug 1255799.

Comment 41

[Tom Tromey :tromey]

Comment on attachment 8726777 [details] [diff] [review]
remove obsolete TODO from network-monitor.js

I'm moving this to bug 1255799.

Comment 42

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8726780 [details] [diff] [review]
add network throttling

Review of attachment 8726780 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/webconsole/network-monitor.js
@@ +502,5 @@
>                                 "http-on-examine-response", false);
>        Services.obs.addObserver(this._httpResponseExaminer,
>                                 "http-on-examine-cached-response", false);
> +      Services.obs.addObserver(this._httpModifyExaminer,
> +                               "http-on-modify-request", false);

The observer should be removed in destroy() method.

Comment 43

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8726782 [details] [diff] [review]
add an API to enable throttling

Review of attachment 8726782 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/netmonitor/test/browser_net_throttle.js
@@ +26,5 @@
> +  let client = monitor._controller.webConsoleClient;
> +
> +  info("sending throttle request");
> +  let deferred = promise.defer();
> +  client.setPreferences(request, response => {

Wouldn't it be better to use WebConsoleFrame.setThrottleData here?

Comment 44

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8726780 [details] [diff] [review]
add network throttling

Review of attachment 8726780 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/shared/webconsole/throttle.js
@@ +199,5 @@
> +   */
> +  start: function(throttleListener) {
> +    this.pendingRequests.add(throttleListener);
> +    setTimeout(() => this.allowDataFrom(throttleListener),
> +               this.random(this.roundTripTimeMean, this.roundTripTimeMax));

Should the timeout be used in cases where the user wants to only slow down downloads? (and keep round trips unaffected)

Comment 45

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am not successful when testing this on real pages.

Since there is no UI yet, I've just appended the following to WebConsoleConnectionProxy
(client/webconsole/webconsole.js)


this.webConsoleFrame.setThrottleData({
  roundTripTimeMean: 1,
  roundTripTimeMax: 1,
  downloadBPSMean: 1024 * 2,
  downloadBPSMax: 1024 * 3,
  uploadBPSMean: 10000,
  uploadBPSMax: 10000,
}).then(response => {
  console.log("Throttle data set ", response);
});


Added just after:
this.webConsoleFrame.setSaveRequestAndResponseBodies(saveBodies);

Does that sound correct?

I couldn't see changes when loading e.g. google.com

Honza

Comment 46

[Jan Honza Odvarko [:Honza] (always need-info? me)]

One additional note related to the previous comment: the Console panel needs to be selected to trigger WebConsoleConnectionProxy._onAttachConsole().

We might want to fine a better place where to configure the throttle feature.

---

I am using this config:
roundTripTimeMean: 1,
roundTripTimeMax: 1,
downloadBPSMean: 1024 * 95,
downloadBPSMax: 1024 * 100,
uploadBPSMean: 10000,
uploadBPSMax: 10000,

... and I am getting too much recursion:

Console Service ERROR [JavaScript Error: "too much recursion" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/throttle.js" line: 215}] [JavaScript Error: "too much recursion" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/throttle.js" line: 215}]


and eventually:
Console Service ERROR [JavaScript Error: "NS_ERROR_INVALID_CONTENT_ENCODING: Component returned failure code: 0x804b001b (NS_ERROR_INVALID_CONTENT_ENCODING) [nsIStreamListener.onDataAvailable]" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js" line: 377}] [JavaScript Error: "NS_ERROR_INVALID_CONTENT_ENCODING: Component returned failure code: 0x804b001b (NS_ERROR_INVALID_CONTENT_ENCODING) [nsIStreamListener.onDataAvailable]" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js" line: 377}]

@Tom: can you reproduce that problem on your machine?

Honza

Comment 47

[Tom Tromey :tromey]

(In reply to Jan Honza Odvarko [:Honza] from comment #46)

> @Tom: can you reproduce that problem on your machine?

Yeah.  I'll look into it.

Comment 48

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Just note for me. Nneeded patches to test the feature, in this order:

bug1255799 (web console eslint)
bug1244227 (add nsIThrottledInputChannel.idl)
bug1244227 (add network throttling)
bug1244227 (add an API to enable throttling)
bug1244227 (remove nsIInterfaceRequestor)
bug1244227 (throttled network timing)

Honza

Comment 49

[Patrick McManus [:mcmanus]]

Comment on attachment 8726778 [details] [diff] [review]
add nsIThrottledInputChannel.idl and implement

Review of attachment 8726778 [details] [diff] [review]:
-----------------------------------------------------------------

I apologize that this fell off my radar - thanks for doing this!

::: netwerk/protocol/http/moz.build
@@ +15,5 @@
>      'nsIHttpChannelInternal.idl',
>      'nsIHttpEventSink.idl',
>      'nsIHttpHeaderVisitor.idl',
>      'nsIHttpProtocolHandler.idl',
> +    'nsIThrottledInputChannel.idl',

I think I would prefer the idl/cpp/h all in base rather than protocol/http

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +373,5 @@
>                                         nsIOService::gDefaultSegmentSize);
>          if (NS_FAILED(rv)) return rv;
>      }
>      else
>          mRequestStream = headers;

can you add MOZ_ASSERT(NS_IsMainThread()) to the top of this function.. that's true of Init() but not generally true of this class.. and its relevant to the code you're adding.

::: netwerk/test/unit/test_throttlechannel.js
@@ +30,5 @@
> +
> +  let tic = channel.QueryInterface(Ci.nsIThrottledInputChannel);
> +  tic.throttleQueue = tq;
> +
> +  channel.asyncOpen2(new ChannelListener(() => {

I think you need to POST a few KB of data for this to be a meaningful test.

::: netwerk/test/unit/test_throttling.js
@@ +26,5 @@
> +
> +  // Approximate size of the request.
> +  let size = 0;
> +  channel.visitRequestHeaders((header, value) => {
> +    size += header.length + value.length;

I think this is pretty sketchy.. I don't want to think too hard about how close this comes to measuring the bytes of the serialzed headers in the input stream, because what I think you really want to test is a request body.. for all practical purposes your rate limiter isn't going to have much to say about headers. test_post.js has an example and that will let you use much higher data amounts and rates.

Comment 50

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #39)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #38)
> 
> > Will this size be the "on the wire" size in the case of gzip encoding, etc.?
> > That's what the previous code was after.
> 
> I will double check to make sure.

It seems to me that this still works correctly.

Using this patch series (without throttling, but with the code in question removed)
I opened bugzilla.mozilla.org.  In the network monitor I see some resources where
the transferred and size fields differ, and clicking on the details I see "gzip"
as the Content-Encoding.

Comment 51

[Tom Tromey :tromey]

Still debugging this but I thought I'd note some of my findings.

Sometimes with the throttler engaged, some downloads don't seem to start.
E.g., if I open bugzilla.mozilla.org, I see 10 downloads work, then activity stops.
Logging shows that new channels simply aren't created.

Interestingly, the problem is related to delaying the data.
If I change the throttler to just pass through calls to it, everything works fine.
(Expected, but still good to verify)

My current theory is that the original nsIStreamListener is not just blindly
consuming data but also examining properties on the nsIRequest object; and
in particular when the data arrives "late" these properties are not consistent
with what is expected.

My next step is to track down exactly what code is being run here and see what is
going wrong.  One option might be to also create a phony (and or wrapper) nsIRequest
to pass on when throttling; but my main concern with this is knowing what other
interfaces such an object might have to implement.

Comment 52

[Tom Tromey :tromey]

A straightforward attempt at wrapping the nsIRequest object failed.
The process crashes with a bad pointer in netwerk code.
I think this is probably not a valid thing to do though I didn't try
to debug exactly why.

Next I'm going to look into platform again to see if I can find out what
exactly is going on.  Maybe one idea would be to build throttling into
platform at the appropriate spot.

Comment 53

[Tom Tromey :tromey]

It's possible that the QI approach had a bug and could somehow be rehabilitated.
To fix the bug we'd need to reimplement the wrapper as a generic Proxy.
However, :jryans found a spot that just static_cast<>s the request pointer, making
me suspect that this is doomed:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#1013

Also there's these assertions:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5854

It's not clear to me if these are upstream or downstream of the throttler; so a bit
more investigation is needed here.


I also found nsIForcePendingChannel, which looked promising.  After changing it to
[scriptable], I can see that it does in fact affect the pending state of the nsIRequest.
However this is insufficient to fix the problem I'm seeing.

I also wonder whether the context object is relevant to any of the downstream listeners,
and if so whether it holds related state that must be hidden.
(Logging it shows it to always be null in my tests; but of course that doesn't say
whether it could be relevant in some scenario.)

Comment 54

[Tom Tromey :tromey]

Reading many ::OnDataAvailable implementations shows that trying to wrap the nsIRequest
will probably not work.  There are many that assume that they can only be called with
a request that they already know about.

I'm now also wondering if we need to implement nsIProgressEventSink and intercept these
events in order to make throttling work properly.

Comment 55

[Tom Tromey :tromey]

Intercepting nsIProgressEventSink did not help.

I did notice that when loading bugzilla.mozilla.org with throttling, there are messages in the console:

ReferenceError: YAHOO is not defined /:25:11
ReferenceError: YAHOO is not defined /:109:1
ReferenceError: YAHOO is not defined /:156:4
ReferenceError: YAHOO is not defined /:174:3

I don't see these without throttling.

Based on this my hypothesis is that some earlier <script> is not being evaluated properly,
and a later <script> is therefore running before it should.
In particular I see:

<script type="text/javascript" src="data/assets/ebf7f6a1b777bdc4cdb7c5177ccfd887.js?1459863067"></script>

    <script type="text/javascript">
    <!--
          YAHOO.namespace('bugzilla');       <==== line 25


So something is going wrong with that first <script>

It also occurred to me that with e10s all the throttling is done in the parent process,
so whatever is going wrong seems like it must necessarily be there.  So, I'm concentrating
my debugging there.

This theory has confused me more than ever, though, because PHttpChannel.ipdl simply doesn't
expose very many methods.  In fact it pretty much looks like a straightforward translation
of nsIProgressEventSink and nsIStreamListener.

Comment 56

[Tom Tromey :tromey]

One curious thing I discovered is that with throttling enabled, some streams will
hit this error return in the child process:

https://dxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/nsHTTPCompressConv.cpp#528

This doesn't happen without throttling, so it seems that somehow the presence
of the throttler changes either the data or whether a converter is instantiated.

Comment 57

[Tom Tromey :tromey]

Something definitely seems wrong in the parent process.  If I log the lengths and initial
bytes in HttpChannelChild::OnTransportAndData I can see discrepancies like:

Working case:

$47 = (nsACString_internal::char_type *) 0x7fc20cf858d8 "https://bugzilla.mozilla.org/data/assets/ebf7f6a1b777bdc4cdb7c5177ccfd887.js?1460468694"
$48 = (const uint32_t &) @0x7fc20d1f6910: 138910
0x7fc20e763008:	0x1f	0x8b	0x08	0x00	0x00	0x00	0x00	0x00


Failing case:

$29 = (nsACString_internal::char_type *) 0x7fc218afe248 "https://bugzilla.mozilla.org/data/assets/ebf7f6a1b777bdc4cdb7c5177ccfd887.js?1460468694"
$30 = (const uint32_t &) @0x7fc20e1bf7d0: 32704
0x7fc20e119008:	0xc6	0x1d	0x97	0xfc	0xaf	0xa0	0xdd	0x1f

Comment 58

[Tom Tromey :tromey]

Ok, I think I found it:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/ArrayBufferInputStream.cpp#114

This passes 0 as the aToOffset argument to the nsWriteSegmentFun.
However, if this has to loop, subsequent buffers will overwrite the first one.
The fix is:

diff --git a/netwerk/base/ArrayBufferInputStream.cpp b/netwerk/base/ArrayBufferInputStream.cpp
index 81bab4b..3526aeb 100644
--- a/netwerk/base/ArrayBufferInputStream.cpp
+++ b/netwerk/base/ArrayBufferInputStream.cpp
@@ -111,7 +111,7 @@ ArrayBufferInputStream::ReadSegments(nsWriteSegmentFun writer, void *closure,
       memcpy(buffer, src, count);
     }
     uint32_t written;
-    nsresult rv = writer(this, closure, buffer, 0, count, &written);
+    nsresult rv = writer(this, closure, buffer, *result, count, &written);
     if (NS_FAILED(rv)) {
       // InputStreams do not propagate errors to caller.
       return NS_OK;

Comment 59

[Tom Tromey :tromey]

There's still some problem -- if I open google.com with the throttler active,
it never seems to complete loading.

Comment 60

[Tom Tromey :tromey]

It happens sometimes, but not always.
And, according to the network monitor, there are the same number of requests in both
cases, and at least there, they all complete as well.

Comment 61

[Tom Tromey :tromey]

I can't reproduce this after rebasing.
This doesn't exactly fill me with hope, but I'm going to go back to addressing
review comments.

Comment 62

[Tom Tromey :tromey]

(In reply to Jan Honza Odvarko [:Honza] from comment #43)
> Comment on attachment 8726782 [details] [diff] [review]
> add an API to enable throttling
> 
> Review of attachment 8726782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/netmonitor/test/browser_net_throttle.js
> @@ +26,5 @@
> > +  let client = monitor._controller.webConsoleClient;
> > +
> > +  info("sending throttle request");
> > +  let deferred = promise.defer();
> > +  client.setPreferences(request, response => {
> 
> Wouldn't it be better to use WebConsoleFrame.setThrottleData here?

Can you tell me how to get that?  I dug around for a while but I didn't see a way.

Comment 63

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Tom Tromey :tromey from comment #62)
> (In reply to Jan Honza Odvarko [:Honza] from comment #43)
> > Comment on attachment 8726782 [details] [diff] [review]
> > add an API to enable throttling
> > 
> > Review of attachment 8726782 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: devtools/client/netmonitor/test/browser_net_throttle.js
> > @@ +26,5 @@
> > > +  let client = monitor._controller.webConsoleClient;
> > > +
> > > +  info("sending throttle request");
> > > +  let deferred = promise.defer();
> > > +  client.setPreferences(request, response => {
> > 
> > Wouldn't it be better to use WebConsoleFrame.setThrottleData here?
> 
> Can you tell me how to get that?  I dug around for a while but I didn't see
> a way.
What I meant was that WebConsoleFrame.setThrottleData API (coming from the 'add an API to enable throttling' patch) could be (perhaps?) used in the test since it's already calls this.webConsoleClient.setPreferences() to setup the backend...

Honza

Comment 64

[Tom Tromey :tromey]

Attachment: Bug 1244227 - add nsIThrottledInputChannel.idl and implement;

Review commit: https://reviewboard.mozilla.org/r/50057/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50057/

Comment 65

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor; r?Honza

Review commit: https://reviewboard.mozilla.org/r/50059/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50059/

Comment 66

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1244227 - add network throttling; r?Honza

Review commit: https://reviewboard.mozilla.org/r/50061/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50061/

Comment 67

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1244227 - properly report throttled network timing; r?Honza

Review commit: https://reviewboard.mozilla.org/r/50063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50063/

Comment 68

[Tom Tromey :tromey]

Attachment: MozReview Request: Bug 1244227 - add an API to enable throttling; r?Honza

Review commit: https://reviewboard.mozilla.org/r/50065/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50065/

Comment 69

[Patrick McManus [:mcmanus]]

Comment on attachment 8747892 [details]
Bug 1244227 - add nsIThrottledInputChannel.idl and implement;

https://reviewboard.mozilla.org/r/50057/#review46821

::: netwerk/base/ThrottleQueue.h:25
(Diff revision 1)
> +  : public nsIInputChannelThrottleQueue
> +  , public nsITimerCallback
> +{
> +public:
> +
> +  ThrottleQueue();

it looks like a throttlequeue can only be used on one thread (the socket thread as that's where the timer events go?). that should be documented and asserted in the class.. especially in methods that touch the arrays which are protected by this property... I would probably also document that we're dealing with a 1 second granularity - so things will be pretty choppy.

::: netwerk/base/ThrottleQueue.h:46
(Diff revision 1)
> +  };
> +
> +  nsTArray<ThrottleEntry> mReadEvents;
> +  uint32_t mMeanBytesPerSecond;
> +  uint32_t mMaxBytesPerSecond;
> +  uint32_t mBytesProcessed;

64bit for total size counter

::: netwerk/protocol/http/HttpBaseChannel.cpp:3316
(Diff revision 1)
> +//-----------------------------------------------------------------------------
> +
> +NS_IMETHODIMP
> +HttpBaseChannel::SetThrottleQueue(nsIInputChannelThrottleQueue* aQueue)
> +{
> +  mThrottleQueue = aQueue;

this should probably fail if a child (content) process tries to do this, right? (otherwise you need code to remote the call to the parent)

::: netwerk/protocol/http/nsHttpTransaction.cpp:12
(Diff revision 1)
>  // HttpLog.h should generally be included first
>  #include "HttpLog.h"
>  
>  #include "base/basictypes.h"
>  
> +#include "nsHttpChannel.h"

I think you want to include some throttle headers, not all of httpChannel, right?

::: netwerk/protocol/http/nsHttpTransaction.cpp:384
(Diff revision 1)
> +    nsCOMPtr<nsIThrottledInputChannel> throttled = do_QueryInterface(mChannel);
> +    nsIInputChannelThrottleQueue* queue;
> +    if (throttled) {
> +        rv = throttled->GetThrottleQueue(&queue);
> +        if (NS_FAILED(rv)) {
> +            return rv;

should this really fail Init()? I would think you would just set queue to null and move on with http

::: netwerk/protocol/http/nsHttpTransaction.cpp:390
(Diff revision 1)
> +        }
> +        if (queue) {
> +            nsIAsyncInputStream* wrappedStream;
> +            rv = queue->WrapStream(mRequestStream, &wrappedStream);
> +            if (NS_FAILED(rv)) {
> +                return rv;

again - probably doesn't need to be fatal

::: netwerk/protocol/http/nsHttpTransaction.cpp:392
(Diff revision 1)
> +            nsIAsyncInputStream* wrappedStream;
> +            rv = queue->WrapStream(mRequestStream, &wrappedStream);
> +            if (NS_FAILED(rv)) {
> +                return rv;
> +            }
> +            mRequestStream = wrappedStream;

defintely want a LOG()

Comment 70

[Tom Tromey :tromey]

Comment on attachment 8747892 [details]
Bug 1244227 - add nsIThrottledInputChannel.idl and implement;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50057/diff/1-2/

Comment 71

[Tom Tromey :tromey]

I sort of thought pushing one patch would leave the others in place.

Comment 72

[Tom Tromey :tromey]

Attachment: Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.

Review commit: https://reviewboard.mozilla.org/r/50825/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50825/

Comment 73

[Tom Tromey :tromey]

Attachment: Bug 1244227 - Add network throttling.

Review commit: https://reviewboard.mozilla.org/r/50827/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50827/

Comment 74

[Tom Tromey :tromey]

Attachment: Bug 1244227 - Properly report throttled network timing.

Review commit: https://reviewboard.mozilla.org/r/50829/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50829/

Comment 75

[Tom Tromey :tromey]

Attachment: Bug 1244227 - Add an API to enable throttling.

Review commit: https://reviewboard.mozilla.org/r/50831/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50831/

Comment 76

[Patrick McManus [:mcmanus]]

Comment on attachment 8747892 [details]
Bug 1244227 - add nsIThrottledInputChannel.idl and implement;

https://reviewboard.mozilla.org/r/50057/#review47773

::: netwerk/protocol/http/HttpBaseChannel.cpp:3344
(Diff revisions 1 - 2)
>  
>  NS_IMETHODIMP
>  HttpBaseChannel::SetThrottleQueue(nsIInputChannelThrottleQueue* aQueue)
>  {
> +  if (!XRE_IsParentProcess()) {
> +    return NS_ERROR_FAILURE;

NS_ERROR_NOT_IMPLEMENTED

Comment 77

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The code looks good to me (my comments from the last time resolved) so, I focused on testing. Btw. I activated the feature by calling `this.webConsoleFrame.setThrottleData` within WebConsoleConnectionProxy._onAttachConsole. 

I tested three scenarios:

1) Download
downloadBPSMean = downloadBPSMax = 50KB/1024KB

2) Upload 
uploadBPSMean = uploadBPSMax = 1024KB

3) Roundtrip
roundTripTimeMean = roundTripTimeMax = 1000ms

--

Download and roundtrip throttling seem to work well, but upload looks broken. I've been uploading 8KB of data, which should take ~8s (uploadBPSMean = uploadBPSMax = 1024KB), but the Network monitor says ~1000ms (and the value appears in the UI after ~1000ms).

In any case, this is great feature!

Honza

Comment 78

[Tom Tromey :tromey]

Not only does upload throttling not seem to work, but now I'm seeing the
"too much recursion" problem again :(

Comment 79

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Tom Tromey :tromey from comment #78)
> Not only does upload throttling not seem to work, but now I'm seeing the
> "too much recursion" problem again :(
Interesting I didn't see it. Do you have specific STR for it or do you see it all the time?
Honza

Comment 80

[Tom Tromey :tromey]

(In reply to Jan Honza Odvarko [:Honza] from comment #79)
> (In reply to Tom Tromey :tromey from comment #78)
> > Not only does upload throttling not seem to work, but now I'm seeing the
> > "too much recursion" problem again :(
> Interesting I didn't see it. Do you have specific STR for it or do you see
> it all the time?

I don't see it all the time, but I can see it if I go to pastebin.mozilla.org
and try to make a new paste -- even something very short like "hello".

Early debugging results seem to show that the nsIPipe that's created in
network-monitor may be calling itself recursively.  Though, it's hard to be
really certain.

Comment 81

[Tom Tromey :tromey]

This seems to happen during a redirect.
The POST to pastebin comes back 302 and then this is invoked:

https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2269

... passing the already-wrapped listener to a new channel.
This is then re-wrapped.  I'm not sure why exactly this causes a problem
(it seems like it should be inefficient but not wrong).

Maybe I can find some way to exclude redirects from extra throttling.

Comment 82

[Tom Tromey :tromey]

In classic rubber duck style, I understood the problem after posting.
The issue is that onDataAvailable will end up calling 'pump', which isn't
reentrant.  Adding a check for this fixes the too much recursion problem.

Comment 83

[Tom Tromey :tromey]

Some updates:

* I found a bug in the platform patch.  It could sometimes request a very long timeout
  due to underflow.
* As far as I can tell, upload throttling works ok and it is the time reporting that
  is wrong
* Rebasing over the XHR stacks patch seems to have introduced new bugs :(

Comment 84

[Tom Tromey :tromey]

The time reporting comes down to this:

https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#1329

I don't understand this line; in particular, why it is looking at "timings.STATUS_SENDING_TO.last".

Replacing this line with:

      harTimings.send = timings.STATUS_SENDING_TO.last - timings.STATUS_SENDING_TO.first;

... makes the timings come out ok.

:Honza, do you know what is going on here?  I thought from the HAR spec that "send" is the
time taken to send the data, so surely status_sending_to.first ought to play into it somehow.

Comment 85

[Tom Tromey :tromey]

We discussed this in person and agreed that the current code is probably incorrect.
I will write a test case using throttling to ensure the new code remains correct.

Comment 86

[Tom Tromey :tromey]

Attachment: Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.

Review commit: https://reviewboard.mozilla.org/r/59818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59818/

Comment 87

[Tom Tromey :tromey]

Comment on attachment 8747892 [details]
Bug 1244227 - add nsIThrottledInputChannel.idl and implement;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50057/diff/2-3/

Comment 88

[Tom Tromey :tromey]

Comment on attachment 8749265 [details]
Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50825/diff/1-2/

Comment 89

[Tom Tromey :tromey]

Comment on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/1-2/

Comment 90

[Tom Tromey :tromey]

Comment on attachment 8749267 [details]
Bug 1244227 - Properly report throttled network timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50829/diff/1-2/

Comment 91

[Tom Tromey :tromey]

Comment on attachment 8749268 [details]
Bug 1244227 - Add an API to enable throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50831/diff/1-2/

Comment 92

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8763651 [details]
Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.

https://reviewboard.mozilla.org/r/59818/#review56864

Comment 93

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8749267 [details]
Bug 1244227 - Properly report throttled network timing.

https://reviewboard.mozilla.org/r/50829/#review56878

::: devtools/shared/webconsole/network-monitor.js
(Diff revision 2)
> -    if ((timings.STATUS_WAITING_FOR || timings.STATUS_RECEIVING_FROM) &&
> -        (timings.STATUS_CONNECTED_TO || timings.STATUS_SENDING_TO)) {
> -      harTimings.send = (timings.STATUS_WAITING_FOR ||
> -                         timings.STATUS_RECEIVING_FROM).first -
> +    if (timings.STATUS_SENDING_TO) {
> +      harTimings.send = timings.STATUS_SENDING_TO.last - timings.STATUS_SENDING_TO.first;
> +    } else if (timings.REQUEST_HEADER && timings.REQUEST_BODY_SENT) {
> +      harTimings.send = timings.REQUEST_BODY_SENT.last - timings.REQUEST_HEADER.first;
> -                        (timings.STATUS_CONNECTED_TO ||
> -                         timings.STATUS_SENDING_TO).last;

I can confirm that this change is correct. Now it works for me (and it doesn't work in the current Nightly)

Comment 94

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

https://reviewboard.mozilla.org/r/50827/#review56884

::: devtools/shared/webconsole/throttle.js:297
(Diff revision 2)
> +  this.downloadQueue =
> +    new NetworkThrottleQueue(downloadBPSMean, downloadBPSMax,
> +                             roundTripTimeMean, roundTripTimeMax);
> +  this.uploadQueue = Cc["@mozilla.org/network/throttlequeue;1"]
> +    .createInstance(Ci.nsIInputChannelThrottleQueue);
> +  this.uploadQueue.init(uploadBPSMean, uploadBPSMax);

if uploadBPSMean/uploadBPSMax is set to 0 this method fires an exception (illegal value). It would be better if 0 means "upload not throttled" but, I am not going to block on this.

Comment 95

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8749268 [details]
Bug 1244227 - Add an API to enable throttling.

https://reviewboard.mozilla.org/r/50831/#review56886

Comment 96

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8749265 [details]
Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.

https://reviewboard.mozilla.org/r/50825/#review56888

Comment 97

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Again, I tested three scenarios:

1) Download
downloadBPSMean = downloadBPSMax = 1024B

2) Upload 
uploadBPSMean = uploadBPSMax = 1024B

3) Roundtrip
roundTripTimeMean = roundTripTimeMax = 1000ms


(btw. in my comment #77, correct values should have been "B" not "KB")


Download, upload and roundtrip seems to be working well for me.

I have also tested the upload timing in the Current Nightly and sending time was really broken, now fixed.

Good job here!

Honza

Comment 98

[Tom Tromey :tromey]

Comment on attachment 8747892 [details]
Bug 1244227 - add nsIThrottledInputChannel.idl and implement;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50057/diff/3-4/

Comment 99

[Tom Tromey :tromey]

Comment on attachment 8749265 [details]
Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50825/diff/2-3/

Comment 100

[Tom Tromey :tromey]

Comment on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/2-3/

Comment 101

[Tom Tromey :tromey]

Comment on attachment 8749267 [details]
Bug 1244227 - Properly report throttled network timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50829/diff/2-3/

Comment 102

[Tom Tromey :tromey]

Comment on attachment 8749268 [details]
Bug 1244227 - Add an API to enable throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50831/diff/2-3/

Comment 103

[Tom Tromey :tromey]

Comment on attachment 8763651 [details]
Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59818/diff/1-2/

Comment 104

[Tom Tromey :tromey]

Comment on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/3-4/

Comment 105

[Tom Tromey :tromey]

Comment on attachment 8749267 [details]
Bug 1244227 - Properly report throttled network timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50829/diff/3-4/

Comment 106

[Tom Tromey :tromey]

Comment on attachment 8749268 [details]
Bug 1244227 - Add an API to enable throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50831/diff/3-4/

Comment 107

[Tom Tromey :tromey]

Comment on attachment 8763651 [details]
Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59818/diff/2-3/

Comment 108

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa2ac096cb97

One of the tests need a little rewording to not timeout on win7.

Comment 109

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/404a39c5e090
add nsIThrottledInputChannel.idl and implement; r=mcmanus
https://hg.mozilla.org/integration/fx-team/rev/e4bf9065b319
NetworkResponseListener does not need to implement nsIInterfaceRequestor; r=Honza
https://hg.mozilla.org/integration/fx-team/rev/2990149167fc
add network throttling; r=Honza
https://hg.mozilla.org/integration/fx-team/rev/b409ab88921d
properly report throttled network timing; r=Honza
https://hg.mozilla.org/integration/fx-team/rev/5855543dc299
add an API to enable throttling; r=Honza
https://hg.mozilla.org/integration/fx-team/rev/fc2dcc9e7f52
remove unused file from devtools/client/netmonitor/test/; r=Honza

Comment 110

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/487b2d6d4ad4 for mochitest-cl failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=10215831&repo=fx-team

Comment 111

[Tom Tromey :tromey]

Comment on attachment 8747892 [details]
Bug 1244227 - add nsIThrottledInputChannel.idl and implement;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50057/diff/4-5/

Comment 112

[Tom Tromey :tromey]

Comment on attachment 8749265 [details]
Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50825/diff/3-4/

Comment 113

[Tom Tromey :tromey]

Comment on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/4-5/

Comment 114

[Tom Tromey :tromey]

Comment on attachment 8749267 [details]
Bug 1244227 - Properly report throttled network timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50829/diff/4-5/

Comment 115

[Tom Tromey :tromey]

Comment on attachment 8749268 [details]
Bug 1244227 - Add an API to enable throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50831/diff/4-5/

Comment 116

[Tom Tromey :tromey]

Comment on attachment 8763651 [details]
Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59818/diff/3-4/

Comment 117

[Tom Tromey :tromey]

I believe the problem was a fencepost error in the test.
It was doing:

      uploadBPSMean: size / 2,
      uploadBPSMax: size / 2,

... and expecting the upload to take more than 2 seconds.  However, the elapsed time
could be less than 2 seconds -- this approach only guarantees more than 1 second,
I believe.

No clue why this only fails on Windows XP.

Try run showing this working

https://treeherder.mozilla.org/#/jobs?repo=try&revision=480fc7643e1a6e318d57348c4537022c2dc3010f&selectedJob=24136536

So, asking for checkin of the updated patches.
The only change is 2->3 in the test.

Comment 118

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5a4bb3258978
Add nsIThrottledInputChannel.idl and implement. r=mcmanus
https://hg.mozilla.org/integration/fx-team/rev/f680f6460f07
NetworkResponseListener does not need to implement nsIInterfaceRequestor. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/33bc49f015a7
Add network throttling. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/49b5309e3415
Properly report throttled network timing. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/d5866b9dd3d9
Add an API to enable throttling. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/0cf1259b7073
Remove unused file from devtools/client/netmonitor/test/. r=Honza

Comment 119

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This broke m(cl) like https://treeherder.mozilla.org/logviewer.html#?job_id=10732752&repo=fx-team

Backed out in https://hg.mozilla.org/integration/fx-team/rev/64e2353eb554

Comment 120

[Tom Tromey :tromey]

This time there's a new exception:

 13:15:07     INFO -  Handler function threw an exception: [Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsISeekableStream.tell]"  nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-helper.js :: NetworkHelper.readPostTextFromRequest :: line 137"  data: no]

 13:15:07     INFO -  Stack: NetworkHelper.readPostTextFromRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-helper.js:137:22

 13:15:07     INFO -  NetworkMonitor.prototype._onRequestBodySent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:1203:20

 13:15:07     INFO -  NetworkMonitor.prototype._dispatchActivity@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:914:9

 13:15:07     INFO -  NetworkMonitor.prototype.observeActivity<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:987:7

 13:15:07     INFO -  exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

13:15:07 INFO - Line: 137, column: 0 


Will investigate next week.

Comment 121

[Tom Tromey :tromey]

Having some trouble reproducing this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d87a2f458c1505d60e5b64a57d12bce4c16c5883

Comment 122

[Tom Tromey :tromey]

Rebasing at least let me reproduce:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7606379d965867149f82bc1c96e9eb955bb85d5e
Will debug more next week.

Comment 123

[Tom Tromey :tromey]

I managed to reproduce it locally.  My current theory is that we're closing
the request stream before we can read it in readPostTextFromRequest.
However, this makes me wonder how this could ever have worked.
I'm looking to see if, when throttling, we can read the post text earlier.

Comment 124

[Tom Tromey :tromey]

That approach worked locally and apparently also on the problem "try" build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cca2c0f36d56626daa18f72330db7b41d79cc22

I'll upload the cleaned-up version and re-run it everywhere to be "sure".

Comment 125

[Tom Tromey :tromey]

Comment on attachment 8749265 [details]
Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50825/diff/4-5/

Comment 126

[Tom Tromey :tromey]

Comment on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/5-6/

Comment 127

[Tom Tromey :tromey]

Comment on attachment 8749267 [details]
Bug 1244227 - Properly report throttled network timing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50829/diff/5-6/

Comment 128

[Tom Tromey :tromey]

Comment on attachment 8749268 [details]
Bug 1244227 - Add an API to enable throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50831/diff/5-6/

Comment 129

[Tom Tromey :tromey]

Comment on attachment 8763651 [details]
Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59818/diff/4-5/

Comment 130

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c339001b416

Comment 131

[Tom Tromey :tromey]

That looks pretty good but I think patch #3 needs a re-review.

Comment 132

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8749267 [details]
Bug 1244227 - Properly report throttled network timing.

https://reviewboard.mozilla.org/r/50829/#review68422

In order to config and test the throttle on real page I am doing the following:

    this.webConsoleFrame.setThrottleData({
      roundTripTimeMean: 1024 * 256,
      roundTripTimeMax: 1024 * 256,
      downloadBPSMean: 1024 * 256,
      downloadBPSMax: 1024 * 256,
      uploadBPSMean: 1024 * 256,
      uploadBPSMax: 1024 * 256,
    }).then(response => {
      console.log("Throttle data set ", response);
    });

I am inserting this code into WebConsoleConnectionProxy._onAttachConsole
(just after setSaveRequestAndResponseBodies)

When testing:

1) Launch Firefox, open DevTools (F12)
2) Select the Console panel so, the code above executes.
3) Select the Network panel, perform XHR and check timings


But, I am experiencing an exception when executing XHR:

JSM error: Handler function threw an exception: TypeError: Cc['@mozilla.org/network/throttlequeue;1'] is undefined
Stack: NetworkThrottleManager@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/throttle.js:370:3
NetworkMonitor.prototype._getThrottler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:753:25
NetworkMonitor.prototype._setupResponseListener@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:1184:23
NetworkMonitor.prototype._createNetworkEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:1080:5
NetworkMonitor.prototype._onRequestHeader@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:1105:5
NetworkMonitor.prototype.observeActivity<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:965:7
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Line: 370, column: 3


--

Btw. I applied 5 patches attached in the bug, nothing else, is that correct?

Not sure if this can be problem on my machine.

The patch looks good code wise, but I'd like to check timings on the Network panel


--

Also, when configuring the throttle feature and testing, it would be useful to set only partial-throttly, e.g. only downlods.
Instead of using 'some huge value' for the other value, it's better to use e.g. -1

So, for example:

    this.webConsoleFrame.setThrottleData({
      roundTripTimeMean: -1,
      roundTripTimeMax: -1,
      downloadBPSMean: 1024 * 256,
      downloadBPSMax: 1024 * 256,
      uploadBPSMean: -1,
      uploadBPSMax: -1,
    })

This would throttle only downloads. Uploads and round-trips would stay unaffected.


Honza

Comment 133

[Tom Tromey :tromey]

(In reply to Jan Honza Odvarko [:Honza] from comment #132)

> But, I am experiencing an exception when executing XHR:
> 
> JSM error: Handler function threw an exception: TypeError:
> Cc['@mozilla.org/network/throttlequeue;1'] is undefined

I haven't been able to reproduce this yet, but I'm seeing another
new bug where the request appears to never complete.

Comment 134

[Tom Tromey :tromey]

(In reply to Tom Tromey :tromey from comment #133)

> I haven't been able to reproduce this yet, but I'm seeing another
> new bug where the request appears to never complete.

I tracked that one down:

    this.webConsoleFrame.setThrottleData({
      roundTripTimeMean: 1024 * 256,
      roundTripTimeMax: 1024 * 256,

... this means wait quite a long time before letting any data through.

Comment 135

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: network-throttle-testing.png

(In reply to Tom Tromey :tromey from comment #133)
> (In reply to Jan Honza Odvarko [:Honza] from comment #132)
> 
> > But, I am experiencing an exception when executing XHR:
> > 
> > JSM error: Handler function threw an exception: TypeError:
> > Cc['@mozilla.org/network/throttlequeue;1'] is undefined
> 
> I haven't been able to reproduce this yet, but I'm seeing another
> new bug where the request appears to never complete.
Sorry, I forgot to apply the "add nsIThrottledInputChannel.idl and implement" patch. Note that it isn't in the list anymore (marked as obsolete)...


(In reply to Tom Tromey :tromey from comment #134)
> (In reply to Tom Tromey :tromey from comment #133)
> 
> > I haven't been able to reproduce this yet, but I'm seeing another
> > new bug where the request appears to never complete.
> 
> I tracked that one down:
> 
>     this.webConsoleFrame.setThrottleData({
>       roundTripTimeMean: 1024 * 256,
>       roundTripTimeMax: 1024 * 256,
> 
> ... this means wait quite a long time before letting any data through.
I see, thanks. Can you please provide quick summary about valid values that can be passed in? Just to make sure I am using the right input values.

---

And now the testing.

It works great if I execute single XHR requests. I can observe that download times are really following the speed I am specifying when calling setThrottleData.

One question though: what is the logic behind the `*Mean` value? How does this affect download time? Can I use it to simulate unstable network speed?


But, I am not convinced that I am getting the right timing results in case of multiple requests happening at the same time.

In my test-case I have a page executing 8 XHRs. It's for 1MB JSON file, browser cache disabled. The page is executing it just after 'load' event is fired.

See the attached screenshot, it shows the network panel (I can also provide HAR files, but they I quite big, including the response. I hope screenshot will be just fine).

1) The first screenshot (top one) shows how it looks like if there is no throttling.
Note that first 6 resources started downloading (the green bar) immediately. The last two had to wait till there is free connection (there is a limit 6 max opened  conn to the server). They started when first and second finished the download.
Also note that the first 6 downloads happen in parallel and they finishes in about the same time.

2) The second screenshot shows how it looks if there is 512Kb/s throttling [1].
Again first six downloads start immediately, but I am not seeing that the last two would wait till some requests finish. Secondly, the first sis downloads doesn't seem to happen in parallel. They don't finish in about the same time. It rather looks that only one download happens at a time ant the other ones are waiting. Only pretending that the download is in-progress, but no receiving any bytes?

Could this be related to how dispatchActivity() is implemented/used?

3) The third screenshot shows how it looks in Chrome (the same scenario + throttling). This is what I would expect. First six downloads happens in parallel, they are done in about the same time and the last two downloads wait till there is a free connection.


Honza

----


[1] Here are the values I am using for 512Kb/s download/upload throttling:

this.webConsoleFrame.setThrottleData({
  roundTripTimeMean: 1,
  roundTripTimeMax: 1,
  downloadBPSMean: 1024 * 512,
  downloadBPSMax: 1024 * 512,
  uploadBPSMean: 1024 * 512,
  uploadBPSMax: 1024 * 512,
})

Comment 136

[Tom Tromey :tromey]

(In reply to Jan Honza Odvarko [:Honza] from comment #135)

> It rather looks that only one download happens at a time ant the
> other ones are waiting. Only pretending that the download is in-progress,
> but no receiving any bytes?

This throttling works by slowing down delivery of bytes to the child process.
It doesn't throttle the actual connections to the server.  So, in a situation
where there's server-side limitations like this, you can indeed see results that
differ from what you'd expect.

We could do that but I think that would require rewriting the download queue
and putting it in platform.  :jryans, what do you think?

Comment 137

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Tom Tromey :tromey from comment #136)
> (In reply to Jan Honza Odvarko [:Honza] from comment #135)
> 
> > It rather looks that only one download happens at a time ant the
> > other ones are waiting. Only pretending that the download is in-progress,
> > but no receiving any bytes?
> 
> This throttling works by slowing down delivery of bytes to the child process.
> It doesn't throttle the actual connections to the server.  So, in a situation
> where there's server-side limitations like this, you can indeed see results
> that
> differ from what you'd expect.
> 
> We could do that but I think that would require rewriting the download queue
> and putting it in platform.  :jryans, what do you think?

From the beginning of this effort, throttling has only been meant as approximation so that you experience your site over a slower connection.  It is not intended to be a precise network analysis tool.

Let's continue to land the current approach and file this issue as future work.

Comment 138

[Tom Tromey :tromey]

Attachment: Add nsIThrottledInputChannel.idl and implement. r=mcmanus

MozReview-Commit-ID: JVIjxEO901W

Comment 139

[Tom Tromey :tromey]

Attachment: NetworkResponseListener does not need to implement nsIInterfaceRequestor. r=Honza

MozReview-Commit-ID: 9TMlmdjQWLL

Comment 140

[Tom Tromey :tromey]

Attachment: Add network throttling. r=Honza

MozReview-Commit-ID: Iy6buFxUrGg

Comment 141

[Tom Tromey :tromey]

Attachment: Properly report throttled network timing. r=Honza

MozReview-Commit-ID: BCJLSRGS0vE

Comment 142

[Tom Tromey :tromey]

Attachment: Add an API to enable throttling. r=Honza

MozReview-Commit-ID: BirjFHVSZN7

Comment 143

[Tom Tromey :tromey]

Attachment: Remove unused file from devtools/client/netmonitor/test/. r=Honza

MozReview-Commit-ID: 1obunKGZYgx

Comment 144

[Tom Tromey :tromey]

Switching back to splinter to work around my mozreview snafu.
I couldn't re-push to mozreview since mcmanus is marked as not accepting reviews
(even though the first patch is r+ already).

Comment 145

[Tom Tromey :tromey]

Comment on attachment 8780567 [details] [diff] [review]
Add network throttling. r=Honza

This version changes the throttling API to disable up- or down-load
throttling if the corresponding parameters are <=0.

Comment 146

[Tom Tromey :tromey]

Comment on attachment 8780569 [details] [diff] [review]
Properly report throttled network timing. r=Honza

This one changed substantially and is the main one needing re-review.

Comment 147

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8780567 [details] [diff] [review]
Add network throttling. r=Honza

Review of attachment 8780567 [details] [diff] [review]:
-----------------------------------------------------------------

> This version changes the throttling API to disable up- or down-load
> throttling if the corresponding parameters are <=0.
Great, thanks!

Honza

Comment 148

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8780569 [details] [diff] [review]
Properly report throttled network timing. r=Honza

Review of attachment 8780569 [details] [diff] [review]:
-----------------------------------------------------------------

Apart from my comment #135, I don't see any code-wise issues, r+

Thanks for working on this!

Honza

Comment 149

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8780570 [details] [diff] [review]
Add an API to enable throttling. r=Honza

Review of attachment 8780570 [details] [diff] [review]:
-----------------------------------------------------------------

Just like in my previous comment, I don't see any issue code-wise.

Honza

Comment 150

[Tom Tromey :tromey]

I am seeing an intermittent timeout on x86 linux debug.
Maybe it's just the try build being slow; I've added some logging and re-pushed a few times to find out.

Comment 151

[Tom Tromey :tromey]

The test that's failing is checking the elapsed time of the entire request (not just
the throttling part but the UI, etc).  I think it can just be slow on this slow build,
and so this part of the test doesn't have much value.  I'm going to remove this; the
test already checks that the throttling works properly, so I think it's fine.

Comment 152

[Tom Tromey :tromey]

Attachment: Add nsIThrottledInputChannel.idl and implement.

MozReview-Commit-ID: JVIjxEO901W

Comment 153

[Tom Tromey :tromey]

Attachment: NetworkResponseListener does not need to implement nsIInterfaceRequestor.

MozReview-Commit-ID: 9TMlmdjQWLL

Comment 154

[Tom Tromey :tromey]

Attachment: Add network throttling.

MozReview-Commit-ID: Iy6buFxUrGg

Comment 155

[Tom Tromey :tromey]

Attachment: Properly report throttled network timing.

MozReview-Commit-ID: BCJLSRGS0vE

Comment 156

[Tom Tromey :tromey]

Attachment: Add an API to enable throttling.

MozReview-Commit-ID: BirjFHVSZN7

Comment 157

[Tom Tromey :tromey]

Attachment: Remove unused file from devtools/client/netmonitor/test/.

MozReview-Commit-ID: 1obunKGZYgx

Comment 158

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da40d503f86a63213b73750936d60d0093749b61

Comment 159

[Tom Tromey :tromey]

I had to update one patch to account for the changes introduce in bug 1240912.

Comment 160

[Tom Tromey :tromey]

Attachment: Add an API to enable throttling.

MozReview-Commit-ID: BirjFHVSZN7

Comment 161

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0193709245a2

Comment 162

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/455c79c657e7
Add nsIThrottledInputChannel.idl and implement. r=mcmanus
https://hg.mozilla.org/integration/fx-team/rev/d2a02407771a
NetworkResponseListener does not need to implement nsIInterfaceRequestor. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/4763f41e1cee
Add network throttling. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/6fc8c6873afa
Properly report throttled network timing. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/8ab89fb64b5c
Add an API to enable throttling. r=Honza
https://hg.mozilla.org/integration/fx-team/rev/76a3bb10b02a
Remove unused file from devtools/client/netmonitor/test/. r=Honza

Comment 163

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/455c79c657e7
https://hg.mozilla.org/mozilla-central/rev/d2a02407771a
https://hg.mozilla.org/mozilla-central/rev/4763f41e1cee
https://hg.mozilla.org/mozilla-central/rev/6fc8c6873afa
https://hg.mozilla.org/mozilla-central/rev/8ab89fb64b5c
https://hg.mozilla.org/mozilla-central/rev/76a3bb10b02a

Comment 164

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:sebo, not sure there's much to document yet, as this bug doesn't add a UI.  Or did you mean for internal docs...?

Comment 165

[Sebastian Zartner [:sebo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #164)
> :sebo, not sure there's much to document yet, as this bug doesn't add a UI. 
> Or did you mean for internal docs...?

I didn't really have a look at the code yet, though it sounded like a reusable code module, so I thought it might be documented at https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules or https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference. If it doesn't fit in those categories, please remove the dev-doc-needed again.

Sebastian

Comment 166

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

AIUI, it's deep in the guts of DevTools, so not very easily reused outside of there.