Closed Bug 1244227 Opened 4 years ago Closed 3 years ago

add back end network speed throttling

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.2 - Aug 29
Tracking Status
firefox51 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Blocks 1 open bug)

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(7 files, 35 obsolete files)

202.55 KB, image/png
Details
33.76 KB, patch
tromey
: review+
Details | Diff | Splinter Review
6.56 KB, patch
tromey
: review+
Details | Diff | Splinter Review
23.52 KB, patch
tromey
: review+
Details | Diff | Splinter Review
19.19 KB, patch
tromey
: review+
Details | Diff | Splinter Review
1.38 KB, patch
tromey
: review+
Details | Diff | Splinter Review
15.66 KB, patch
tromey
: review+
Details | Diff | Splinter Review
Add back end support for the network throttling control project.
See bug 1110207 for the user story, bug 1239925 for the breakdown bug.
Attached patch P (obsolete) — Splinter Review
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.
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.
(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.
FWIW I tend to think the "offline" case should be implemented differently.
Bug 1156659 could be used for this.
Platform support for upload throttling.
Attachment #8713742 - Attachment is obsolete: true
Update throttler to handle uploads as well.
(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.
FWIW now I think that upload interception patch is just completely wrong.
MozReview-Commit-ID: AnaXFFHlNLM
Attachment #8714912 - Attachment is obsolete: true
Attachment #8714914 - Attachment is obsolete: true
MozReview-Commit-ID: Lbx8C41oSqG
MozReview-Commit-ID: JVIjxEO901W

This is a new approach to throttling the request.
MozReview-Commit-ID: 9TMlmdjQWLL
Attached patch add network throttling (obsolete) — Splinter Review
MozReview-Commit-ID: Iy6buFxUrGg
MozReview-Commit-ID: BCJLSRGS0vE
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 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.
Attachment #8722689 - Flags: feedback?(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
Attachment #8722689 - Flags: feedback?(mcmanus)
(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
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 :(
(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.
Blocks: 1110207
Whiteboard: [multiviewport]
MozReview-Commit-ID: AnaXFFHlNLM
Attachment #8722685 - Attachment is obsolete: true
Attachment #8722686 - Attachment is obsolete: true
Attachment #8722689 - Attachment is obsolete: true
Attachment #8722692 - Attachment is obsolete: true
Attachment #8722693 - Attachment is obsolete: true
Attachment #8722694 - Attachment is obsolete: true
MozReview-Commit-ID: Lbx8C41oSqG
MozReview-Commit-ID: JVIjxEO901W
MozReview-Commit-ID: 9TMlmdjQWLL
Attached patch add network throttling (obsolete) — Splinter Review
MozReview-Commit-ID: Iy6buFxUrGg
MozReview-Commit-ID: BCJLSRGS0vE
Attached patch add an API to enable throttling (obsolete) — Splinter Review
MozReview-Commit-ID: BirjFHVSZN7
Comment on attachment 8726776 [details] [diff] [review]
make network-monitor eslint-clean

This patch is a reasonably straightforward eslint cleanup of network-monitor.js.
Attachment #8726776 - Flags: review?(odvarko)
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.
Attachment #8726777 - Flags: review?(odvarko)
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.
Attachment #8726778 - Flags: review?(mcmanus)
After rebasing I noticed one new eslint warning.

MozReview-Commit-ID: AnaXFFHlNLM
Attachment #8726776 - Attachment is obsolete: true
Attachment #8726776 - Flags: review?(odvarko)
Comment on attachment 8726848 [details] [diff] [review]
make network-monitor eslint-clean

This patch is a reasonably straightforward eslint cleanup of network-monitor.js.
Attachment #8726848 - Flags: review?(odvarko)
Minor cleanup to previous version of this patch.

MozReview-Commit-ID: 9TMlmdjQWLL
Attachment #8726779 - Attachment is obsolete: true
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.
Attachment #8726850 - Flags: review?(odvarko)
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.
Attachment #8726780 - Flags: review?(odvarko)
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.
Attachment #8726781 - Flags: review?(odvarko)
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.
Attachment #8726782 - Flags: review?(odvarko)
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.
(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.
Flags: qe-verify?
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Flags: qe-verify? → qe-verify-
Comment on attachment 8726848 [details] [diff] [review]
make network-monitor eslint-clean

I'm moving this to bug 1255799.
Attachment #8726848 - Attachment is obsolete: true
Attachment #8726848 - Flags: review?(odvarko)
Comment on attachment 8726777 [details] [diff] [review]
remove obsolete TODO from network-monitor.js

I'm moving this to bug 1255799.
Attachment #8726777 - Attachment is obsolete: true
Attachment #8726777 - Flags: review?(odvarko)
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.
Attachment #8726780 - Flags: review?(odvarko) → review-
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 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)
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
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
(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.
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 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.
Attachment #8726778 - Flags: review?(mcmanus) → review+
(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.
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.
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.
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.)
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.
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.
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.
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
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;
Depends on: 1265076
There's still some problem -- if I open google.com with the throttler active,
it never seems to complete loading.
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.
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.
(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.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Review commit: https://reviewboard.mozilla.org/r/50057/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50057/
Attachment #8747892 - Flags: review?(mcmanus)
Attachment #8747893 - Flags: review?(odvarko)
Attachment #8747894 - Flags: review?(odvarko)
Attachment #8747895 - Flags: review?(odvarko)
Attachment #8747896 - Flags: review?(odvarko)
Attachment #8726778 - Attachment is obsolete: true
Attachment #8726780 - Attachment is obsolete: true
Attachment #8726781 - Attachment is obsolete: true
Attachment #8726781 - Flags: review?(odvarko)
Attachment #8726782 - Attachment is obsolete: true
Attachment #8726782 - Flags: review?(odvarko)
Attachment #8726850 - Attachment is obsolete: true
Attachment #8726850 - Flags: review?(odvarko)
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()
Attachment #8747892 - Flags: review?(mcmanus)
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/
Attachment #8747892 - Flags: review?(mcmanus)
Attachment #8747893 - Attachment is obsolete: true
Attachment #8747893 - Flags: review?(odvarko)
Attachment #8747894 - Attachment is obsolete: true
Attachment #8747894 - Flags: review?(odvarko)
Attachment #8747895 - Attachment is obsolete: true
Attachment #8747895 - Flags: review?(odvarko)
Attachment #8747896 - Attachment is obsolete: true
Attachment #8747896 - Flags: review?(odvarko)
I sort of thought pushing one patch would leave the others in place.
Review commit: https://reviewboard.mozilla.org/r/50825/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50825/
Attachment #8749265 - Flags: review?(odvarko)
Attachment #8749266 - Flags: review?(odvarko)
Attachment #8749267 - Flags: review?(odvarko)
Attachment #8749268 - Flags: review?(odvarko)
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
Attachment #8747892 - Flags: review?(mcmanus) → review+
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
Not only does upload throttling not seem to work, but now I'm seeing the
"too much recursion" problem again :(
(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
(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.
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.
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.
Priority: -- → P2
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 :(
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.
Flags: needinfo?(odvarko)
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.
Flags: needinfo?(odvarko)
Review commit: https://reviewboard.mozilla.org/r/59818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59818/
Attachment #8747892 - Attachment description: MozReview Request: Bug 1244227 - add nsIThrottledInputChannel.idl and implement; r?mcmanus → Bug 1244227 - add nsIThrottledInputChannel.idl and implement;
Attachment #8749265 - Attachment description: MozReview Request: Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor; r?Honza → Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor;
Attachment #8749266 - Attachment description: MozReview Request: Bug 1244227 - add network throttling; r?Honza → Bug 1244227 - add network throttling;
Attachment #8749267 - Attachment description: MozReview Request: Bug 1244227 - properly report throttled network timing; r?Honza → Bug 1244227 - properly report throttled network timing;
Attachment #8749268 - Attachment description: MozReview Request: Bug 1244227 - add an API to enable throttling; r?Honza → Bug 1244227 - add an API to enable throttling;
Attachment #8763651 - Flags: review?(odvarko)
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 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 on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/1-2/
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 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 on attachment 8763651 [details]
Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.

https://reviewboard.mozilla.org/r/59818/#review56864
Attachment #8763651 - Flags: review?(odvarko) → review+
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)
Attachment #8749267 - Flags: review?(odvarko) → review+
Attachment #8749266 - Flags: review?(odvarko) → review+
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 on attachment 8749265 [details]
Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.

https://reviewboard.mozilla.org/r/50825/#review56888
Attachment #8749265 - Flags: review?(odvarko) → review+
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 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 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 on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/2-3/
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 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 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 on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/3-4/
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 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 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/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa2ac096cb97

One of the tests need a little rewording to not timeout on win7.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
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 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 on attachment 8749266 [details]
Bug 1244227 - Add network throttling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50827/diff/4-5/
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 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 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/
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.
Flags: needinfo?(ttromey)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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.
Flags: needinfo?(ttromey)
Rebasing at least let me reproduce:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7606379d965867149f82bc1c96e9eb955bb85d5e
Will debug more next week.
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.
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 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/
Attachment #8749265 - Attachment description: Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor; → Bug 1244227 - NetworkResponseListener does not need to implement nsIInterfaceRequestor.
Attachment #8749266 - Attachment description: Bug 1244227 - add network throttling; → Bug 1244227 - Add network throttling.
Attachment #8749267 - Attachment description: Bug 1244227 - properly report throttled network timing; → Bug 1244227 - Properly report throttled network timing.
Attachment #8749268 - Attachment description: Bug 1244227 - add an API to enable throttling; → Bug 1244227 - Add an API to enable throttling.
Attachment #8763651 - Attachment description: Bug 1244227 - remove unused file from devtools/client/netmonitor/test/; → Bug 1244227 - Remove unused file from devtools/client/netmonitor/test/.
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 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 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 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/
Attachment #8747892 - Attachment is obsolete: true
That looks pretty good but I think patch #3 needs a re-review.
Flags: needinfo?(odvarko)
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
Attachment #8749267 - Flags: review+ → review-
(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.
(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.
(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,
})
Flags: needinfo?(odvarko)
(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?
Flags: needinfo?(jryans)
(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.
Flags: needinfo?(jryans)
MozReview-Commit-ID: JVIjxEO901W
MozReview-Commit-ID: 9TMlmdjQWLL
Attached patch Add network throttling. r=Honza (obsolete) — Splinter Review
MozReview-Commit-ID: Iy6buFxUrGg
MozReview-Commit-ID: BCJLSRGS0vE
MozReview-Commit-ID: BirjFHVSZN7
MozReview-Commit-ID: 1obunKGZYgx
Attachment #8749265 - Attachment is obsolete: true
Attachment #8749266 - Attachment is obsolete: true
Attachment #8749267 - Attachment is obsolete: true
Attachment #8749268 - Attachment is obsolete: true
Attachment #8763651 - Attachment is obsolete: true
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).
Attachment #8780565 - Flags: review+
Attachment #8780566 - Flags: review+
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.
Attachment #8780567 - Flags: review?(odvarko)
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.
Attachment #8780569 - Flags: review?(odvarko)
Attachment #8780570 - Flags: review?(odvarko)
Attachment #8780571 - Flags: review+
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
Attachment #8780567 - Flags: review?(odvarko) → review+
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
Attachment #8780569 - Flags: review?(odvarko) → review+
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
Attachment #8780570 - Flags: review?(odvarko) → review+
Iteration: --- → 51.2 - Aug 29
Priority: P2 → P1
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.
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.
MozReview-Commit-ID: JVIjxEO901W
Attachment #8780565 - Attachment is obsolete: true
Attachment #8780566 - Attachment is obsolete: true
Attachment #8780567 - Attachment is obsolete: true
Attachment #8780569 - Attachment is obsolete: true
Attachment #8780570 - Attachment is obsolete: true
Attachment #8780571 - Attachment is obsolete: true
MozReview-Commit-ID: 9TMlmdjQWLL
MozReview-Commit-ID: Iy6buFxUrGg
MozReview-Commit-ID: BCJLSRGS0vE
Attached patch Add an API to enable throttling. (obsolete) — Splinter Review
MozReview-Commit-ID: BirjFHVSZN7
MozReview-Commit-ID: 1obunKGZYgx
Attachment #8783540 - Flags: review+
Attachment #8783541 - Flags: review+
Attachment #8783542 - Flags: review+
Attachment #8783544 - Flags: review+
Attachment #8783545 - Flags: review+
Attachment #8783546 - Flags: review+
I had to update one patch to account for the changes introduce in bug 1240912.
MozReview-Commit-ID: BirjFHVSZN7
Attachment #8783545 - Attachment is obsolete: true
Attachment #8783605 - Flags: review+
Pushed by ttromey@mozilla.com:
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
:sebo, not sure there's much to document yet, as this bug doesn't add a UI.  Or did you mean for internal docs...?
Flags: needinfo?(sebastianzartner)
(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
Flags: needinfo?(sebastianzartner)
AIUI, it's deep in the guts of DevTools, so not very easily reused outside of there.
Keywords: dev-doc-needed
Depends on: 1309523
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.