add back end network speed throttling

RESOLVED FIXED in Firefox 51

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 51
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 fixed)

Details

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

Attachments

(7 attachments, 35 obsolete attachments)

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
(Assignee)

Description

2 years ago
Add back end support for the network throttling control project.
See bug 1110207 for the user story, bug 1239925 for the breakdown bug.
(Assignee)

Comment 1

2 years ago
Created attachment 8713742 [details] [diff] [review]
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.
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
(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.
(Assignee)

Comment 4

2 years ago
FWIW I tend to think the "offline" case should be implemented differently.
Bug 1156659 could be used for this.
(Assignee)

Comment 5

2 years ago
Created attachment 8714912 [details] [diff] [review]
nsITraceableChannel.setNewUploadListener

Platform support for upload throttling.
Attachment #8713742 - Attachment is obsolete: true
(Assignee)

Comment 6

2 years ago
Created attachment 8714914 [details] [diff] [review]
add network throttling for downloads

Update throttler to handle uploads as well.
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
FWIW now I think that upload interception patch is just completely wrong.
(Assignee)

Comment 9

2 years ago
Created attachment 8722685 [details] [diff] [review]
make network-monitor eslint-clean

MozReview-Commit-ID: AnaXFFHlNLM
Attachment #8714912 - Attachment is obsolete: true
Attachment #8714914 - Attachment is obsolete: true
(Assignee)

Comment 10

2 years ago
Created attachment 8722686 [details] [diff] [review]
remove obsolete TODO from network-monitor.js

MozReview-Commit-ID: Lbx8C41oSqG
(Assignee)

Comment 11

2 years ago
Created attachment 8722689 [details] [diff] [review]
add nsIHttpChannel.setUploadThrottleRate

MozReview-Commit-ID: JVIjxEO901W

This is a new approach to throttling the request.
(Assignee)

Comment 12

2 years ago
Created attachment 8722692 [details] [diff] [review]
NetworkResponseListener does not need to implement nsIInterfaceRequestor

MozReview-Commit-ID: 9TMlmdjQWLL
(Assignee)

Comment 13

2 years ago
Created attachment 8722693 [details] [diff] [review]
add network throttling

MozReview-Commit-ID: Iy6buFxUrGg
(Assignee)

Comment 14

2 years ago
Created attachment 8722694 [details] [diff] [review]
properly report throttled network timing

MozReview-Commit-ID: BCJLSRGS0vE
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
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)
(Assignee)

Comment 18

2 years ago
(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 :(
(Assignee)

Comment 20

2 years ago
(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.

Updated

2 years ago
Blocks: 1110207
Whiteboard: [multiviewport]
(Assignee)

Comment 21

2 years ago
Created attachment 8726776 [details] [diff] [review]
make network-monitor eslint-clean

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
(Assignee)

Comment 22

2 years ago
Created attachment 8726777 [details] [diff] [review]
remove obsolete TODO from network-monitor.js

MozReview-Commit-ID: Lbx8C41oSqG
(Assignee)

Comment 23

2 years ago
Created attachment 8726778 [details] [diff] [review]
add nsIThrottledInputChannel.idl and implement

MozReview-Commit-ID: JVIjxEO901W
(Assignee)

Comment 24

2 years ago
Created attachment 8726779 [details] [diff] [review]
NetworkResponseListener does not need to implement nsIInterfaceRequestor

MozReview-Commit-ID: 9TMlmdjQWLL
(Assignee)

Comment 25

2 years ago
Created attachment 8726780 [details] [diff] [review]
add network throttling

MozReview-Commit-ID: Iy6buFxUrGg
(Assignee)

Comment 26

2 years ago
Created attachment 8726781 [details] [diff] [review]
properly report throttled network timing

MozReview-Commit-ID: BCJLSRGS0vE
(Assignee)

Comment 27

2 years ago
Created attachment 8726782 [details] [diff] [review]
add an API to enable throttling

MozReview-Commit-ID: BirjFHVSZN7
(Assignee)

Comment 28

2 years ago
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)
(Assignee)

Comment 29

2 years ago
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)
(Assignee)

Comment 30

2 years ago
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)
(Assignee)

Comment 31

2 years ago
Created attachment 8726848 [details] [diff] [review]
make network-monitor eslint-clean

After rebasing I noticed one new eslint warning.

MozReview-Commit-ID: AnaXFFHlNLM
Attachment #8726776 - Attachment is obsolete: true
Attachment #8726776 - Flags: review?(odvarko)
(Assignee)

Comment 32

2 years ago
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)
(Assignee)

Comment 33

2 years ago
Created attachment 8726850 [details] [diff] [review]
NetworkResponseListener does not need to implement nsIInterfaceRequestor

Minor cleanup to previous version of this patch.

MozReview-Commit-ID: 9TMlmdjQWLL
Attachment #8726779 - Attachment is obsolete: true
(Assignee)

Comment 34

2 years ago
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)
(Assignee)

Comment 35

2 years ago
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)
(Assignee)

Comment 36

2 years ago
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)
(Assignee)

Comment 37

2 years ago
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.
(Assignee)

Comment 39

2 years ago
(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.

Updated

2 years ago
Flags: qe-verify?
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Flags: qe-verify? → qe-verify-
(Assignee)

Comment 40

2 years ago
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)
(Assignee)

Comment 41

2 years ago
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
(Assignee)

Comment 47

2 years ago
(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+
(Assignee)

Comment 50

2 years ago
(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.
(Assignee)

Comment 51

2 years ago
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.
(Assignee)

Comment 52

2 years ago
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.
(Assignee)

Comment 53

2 years ago
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.)
(Assignee)

Comment 54

2 years ago
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.
(Assignee)

Comment 55

2 years ago
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.
(Assignee)

Comment 56

2 years ago
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.
(Assignee)

Comment 57

2 years ago
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
(Assignee)

Comment 58

2 years ago
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;
(Assignee)

Updated

2 years ago
Depends on: 1265076
(Assignee)

Comment 59

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

Comment 60

2 years ago
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.
(Assignee)

Comment 61

2 years ago
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.
(Assignee)

Comment 62

2 years ago
(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)
(Assignee)

Comment 64

2 years ago
Created attachment 8747892 [details]
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/
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)
(Assignee)

Comment 65

2 years ago
Created attachment 8747893 [details]
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/
(Assignee)

Comment 66

2 years ago
Created attachment 8747894 [details]
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/
(Assignee)

Comment 67

2 years ago
Created attachment 8747895 [details]
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/
(Assignee)

Comment 68

2 years ago
Created attachment 8747896 [details]
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/
(Assignee)

Updated

2 years ago
Attachment #8726778 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8726780 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8726781 - Attachment is obsolete: true
Attachment #8726781 - Flags: review?(odvarko)
(Assignee)

Updated

2 years ago
Attachment #8726782 - Attachment is obsolete: true
Attachment #8726782 - Flags: review?(odvarko)
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 70

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8747893 - Attachment is obsolete: true
Attachment #8747893 - Flags: review?(odvarko)
(Assignee)

Updated

2 years ago
Attachment #8747894 - Attachment is obsolete: true
Attachment #8747894 - Flags: review?(odvarko)
(Assignee)

Updated

2 years ago
Attachment #8747895 - Attachment is obsolete: true
Attachment #8747895 - Flags: review?(odvarko)
(Assignee)

Updated

2 years ago
Attachment #8747896 - Attachment is obsolete: true
Attachment #8747896 - Flags: review?(odvarko)
(Assignee)

Comment 71

2 years ago
I sort of thought pushing one patch would leave the others in place.
(Assignee)

Comment 72

2 years ago
Created attachment 8749265 [details]
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/
Attachment #8749265 - Flags: review?(odvarko)
Attachment #8749266 - Flags: review?(odvarko)
Attachment #8749267 - Flags: review?(odvarko)
Attachment #8749268 - Flags: review?(odvarko)
(Assignee)

Comment 74

2 years ago
Created attachment 8749267 [details]
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/
(Assignee)

Comment 75

2 years ago
Created attachment 8749268 [details]
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 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
(Assignee)

Comment 78

2 years ago
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
(Assignee)

Comment 80

2 years ago
(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.
(Assignee)

Comment 81

2 years ago
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.
(Assignee)

Comment 82

2 years ago
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.

Updated

2 years ago
Priority: -- → P2
(Assignee)

Comment 83

2 years ago
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 :(
(Assignee)

Comment 84

2 years ago
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)
(Assignee)

Comment 85

2 years ago
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)
(Assignee)

Comment 86

2 years ago
Created attachment 8763651 [details]
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/
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)
(Assignee)

Comment 87

2 years ago
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/
(Assignee)

Comment 88

2 years ago
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/
(Assignee)

Comment 89

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

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

Comment 90

2 years ago
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/
(Assignee)

Comment 91

2 years ago
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 8749268 [details]
Bug 1244227 - Add an API to enable throttling.

https://reviewboard.mozilla.org/r/50831/#review56886
Attachment #8749268 - Flags: review?(odvarko) → review+
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
(Assignee)

Comment 98

2 years ago
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/
(Assignee)

Comment 99

2 years ago
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/
(Assignee)

Comment 100

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

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

Comment 101

2 years ago
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/
(Assignee)

Comment 102

2 years ago
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/
(Assignee)

Comment 103

2 years ago
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/
(Assignee)

Comment 104

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

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

Comment 105

2 years ago
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/
(Assignee)

Comment 106

2 years ago
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/
(Assignee)

Comment 107

2 years ago
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/
(Assignee)

Comment 108

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

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

Updated

2 years ago
Keywords: checkin-needed

Comment 109

2 years ago
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
(Assignee)

Comment 111

2 years ago
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/
(Assignee)

Comment 112

2 years ago
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/
(Assignee)

Comment 113

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

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

Comment 114

2 years ago
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/
(Assignee)

Comment 115

2 years ago
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/
(Assignee)

Comment 116

2 years ago
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/
(Assignee)

Comment 117

2 years ago
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

Comment 118

2 years ago
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
(Assignee)

Comment 120

2 years ago
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)
(Assignee)

Comment 122

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

Comment 123

2 years ago
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.
(Assignee)

Comment 124

2 years ago
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".
(Assignee)

Comment 125

2 years ago
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/.
(Assignee)

Comment 126

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

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

Comment 127

2 years ago
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/
(Assignee)

Comment 128

2 years ago
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/
(Assignee)

Comment 129

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8747892 - Attachment is obsolete: true
(Assignee)

Comment 131

2 years ago
That looks pretty good but I think patch #3 needs a re-review.
Flags: needinfo?(odvarko)

Comment 132

2 years ago
mozreview-review
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-
(Assignee)

Comment 133

2 years ago
(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.
(Assignee)

Comment 134

2 years ago
(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.
Created attachment 8780427 [details]
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,
})
Flags: needinfo?(odvarko)
(Assignee)

Comment 136

2 years ago
(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)
(Assignee)

Comment 138

2 years ago
Created attachment 8780565 [details] [diff] [review]
Add nsIThrottledInputChannel.idl and implement. r=mcmanus

MozReview-Commit-ID: JVIjxEO901W
(Assignee)

Comment 139

2 years ago
Created attachment 8780566 [details] [diff] [review]
NetworkResponseListener does not need to implement nsIInterfaceRequestor. r=Honza

MozReview-Commit-ID: 9TMlmdjQWLL
(Assignee)

Comment 140

2 years ago
Created attachment 8780567 [details] [diff] [review]
Add network throttling. r=Honza

MozReview-Commit-ID: Iy6buFxUrGg
(Assignee)

Comment 141

2 years ago
Created attachment 8780569 [details] [diff] [review]
Properly report throttled network timing. r=Honza

MozReview-Commit-ID: BCJLSRGS0vE
(Assignee)

Comment 142

2 years ago
Created attachment 8780570 [details] [diff] [review]
Add an API to enable throttling. r=Honza

MozReview-Commit-ID: BirjFHVSZN7
(Assignee)

Comment 143

2 years ago
Created attachment 8780571 [details] [diff] [review]
Remove unused file from devtools/client/netmonitor/test/. r=Honza

MozReview-Commit-ID: 1obunKGZYgx
(Assignee)

Updated

2 years ago
Attachment #8749265 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8749266 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8749267 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8749268 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8763651 - Attachment is obsolete: true
(Assignee)

Comment 144

2 years ago
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).
(Assignee)

Updated

2 years ago
Attachment #8780565 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8780566 - Flags: review+
(Assignee)

Comment 145

2 years ago
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)
(Assignee)

Comment 146

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8780570 - Flags: review?(odvarko)
(Assignee)

Updated

2 years ago
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+

Updated

2 years ago
Iteration: --- → 51.2 - Aug 29
Priority: P2 → P1
(Assignee)

Comment 150

2 years ago
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.
(Assignee)

Comment 151

2 years ago
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.
(Assignee)

Comment 152

2 years ago
Created attachment 8783540 [details] [diff] [review]
Add nsIThrottledInputChannel.idl and implement.

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
(Assignee)

Comment 153

2 years ago
Created attachment 8783541 [details] [diff] [review]
NetworkResponseListener does not need to implement nsIInterfaceRequestor.

MozReview-Commit-ID: 9TMlmdjQWLL
(Assignee)

Comment 154

2 years ago
Created attachment 8783542 [details] [diff] [review]
Add network throttling.

MozReview-Commit-ID: Iy6buFxUrGg
(Assignee)

Comment 155

2 years ago
Created attachment 8783544 [details] [diff] [review]
Properly report throttled network timing.

MozReview-Commit-ID: BCJLSRGS0vE
(Assignee)

Comment 156

2 years ago
Created attachment 8783545 [details] [diff] [review]
Add an API to enable throttling.

MozReview-Commit-ID: BirjFHVSZN7
(Assignee)

Comment 157

2 years ago
Created attachment 8783546 [details] [diff] [review]
Remove unused file from devtools/client/netmonitor/test/.

MozReview-Commit-ID: 1obunKGZYgx
(Assignee)

Updated

2 years ago
Attachment #8783540 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8783541 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8783542 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8783544 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8783545 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8783546 - Flags: review+
(Assignee)

Comment 159

2 years ago
I had to update one patch to account for the changes introduce in bug 1240912.
(Assignee)

Comment 160

2 years ago
Created attachment 8783605 [details] [diff] [review]
Add an API to enable throttling.

MozReview-Commit-ID: BirjFHVSZN7
Attachment #8783545 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8783605 - Flags: review+

Comment 162

2 years ago
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
Keywords: dev-doc-needed
: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

Updated

2 years ago
Depends on: 1309523
You need to log in before you can comment on or make changes to this bug.