Closed Bug 1007020 Opened 5 years ago Closed 5 years ago

XHR Progress Events need a way to read how many compressed bytes have been downloaded

Categories

(Core :: Networking: HTTP, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: hsteen, Assigned: dragana)

References

Details

Attachments

(2 files, 12 obsolete files)

8.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.48 KB, patch
dragana
: review+
Details | Diff | Splinter Review
Progress events will have a .total property showing the value of Content-Length (if sent by the server) and a .loaded property showing how much data has been fetched already. For gzipped data, the current implementation sets .loaded to the number of bytes *after* decompression, this is wrong. To be meaningfully compared with .total it needs to be the number of bytes that came over the wire before decompression.

Per bug 614352, Necko doesn't currently expose this number - please do if you don't already..
Blocks: xhr2pass
Jonas, what's the necko API change that we need here?

Is the issue that at the necko level is that we provide nsIProgressEventSink with a 'progress' and 'progressMax' values that are based on uncompressed and and compressed (Content-Length header) values? If so I assume the fix is to add some "compressedProgress" value (or change 'progress' to be compressed and add an "uncompressedProgress" value).

But I'm not 100% sure this is how XHR is getting the progress info here.
Component: Networking → Networking: HTTP
necko provides nsIProgressEventSink and sends 'progress' and 'progressMax' both for comprassed data. 
So we do not need to change anything here.

progress uncompressed data, the number of bytes *after* decompression, are send in OnDataAvailable

XHR can get progress values, using OnProgress events only if flag nsIRequest::LOAD_BACKGROUND is unset.
in nsXMLHttpRequest.cpp there is a comment explaining this:


2658   // nsIRequest::LOAD_BACKGROUND prevents throbber from becoming active, which
2659   // in turn keeps STOP button from becoming active.  If the consumer passed in
2660   // a progress event handler we must load with nsIRequest::LOAD_NORMAL or
2661   // necko won't generate any progress notifications.
2662   if (HasListenersFor(nsGkAtoms::onprogress) ||
2663       (mUpload && mUpload->HasListenersFor(nsGkAtoms::onprogress))) {
2664     nsLoadFlags loadFlags;
2665     mChannel->GetLoadFlags(&loadFlags);
2666     loadFlags &= ~nsIRequest::LOAD_BACKGROUND;
2667     loadFlags |= nsIRequest::LOAD_NORMAL;
2668     mChannel->SetLoadFlags(loadFlags);
2669   }

in nsXMLHttpRequest.cpp, function  DispatchProgressEvent sends progress data for decompressed data, actually information about compressed data progress is not saved in any member variable.
> So we do not need to change anything here.

How does "progress" interact with chunked transfer-encoding for HTTP?

> only if flag nsIRequest::LOAD_BACKGROUND is unset

Yes, and XHR needs that flag to be set so it doesn't trigger UI indicators of network activity.  In fact, the fact that we have to not set that flag to get progress events is part of what I would like necko to fix here.
Flags: needinfo?(dd.mozilla)
> How does "progress" interact with chunked transfer-encoding for HTTP?

Good question--we need to figure that out.

> the fact that we have to not set that flag to get progress events is part of 
> what I would like necko to fix here.

It's a one-line fix to change this in necko:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5442

The issue is whether it will break things above it.  It sounds from "nsIRequest::LOAD_BACKGROUND prevents throbber from becoming active" that there's some code in docshell/etc that's assuming receiving OnProgress means "turn the throbber on"?  If so that code is going to need to change (maybe to query the load flags of the nsIRequest to see if LOAD_BACKGROUND is set?).  Is there anything else that could break if we start dispatching OnProgress for background requests?
Flags: needinfo?(dd.mozilla)
I'd be pretty wary of what addon expectations are around background requests and progress notifications.  But yes, in general there's docshell code that watches all requests and then sends those notifications to browser code; the browser code is what manages the throbber...  That code actually goes off start/stop notifications, iirc, not progress ones, but it might also examine the LOAD_BACKGROUND flag and ignore channels that have it.  So what LOAD_BACKGROUND buys us there is the explicit checks in docshell, not the suppression of progress events.

So maybe just sending progress in necko for LOAD_BACKGROUND is ok.

If we think it's not, one option that would work for XHR is to dispatch the progress bits to the _channel_ callbacks no matter what, but only notify the loadgroup callbacks if not LOAD_BACKGROUND.
> So maybe just sending progress in necko for LOAD_BACKGROUND is ok.  If we
> think it's not, one option that would work for XHR is to dispatch the progress
> bits to the _channel_ callbacks no matter what, but only notify the loadgroup
> callbacks if not LOAD_BACKGROUND.

I don't see any code in nsLoadGroup.cpp that handles OnProgress (and no other code seems to implement nsILoadGroup), so that makes me think there's no particular benefit to special-casing the OnProgress to just channel callbacks.

I haven't audited OnProgress call sites in general to see if firing it for BACKGROUND requests would mess things up.  Nor have I looked at addons yet: looks like we have 97 that are known to refer to LOAD_BACKGROUND:

   https://mxr.mozilla.org/addons/search?string=LOAD_BACKGROUND&find=&findi=&filter=^[^\0]*$&hitlimit=&tree=addons

about chunked transfer-encoding:
progressMax is set to UINT64_MAX and progress is update the same as with no encoding.
I am trying to change OnProgress behaviour. I have problem with test ./content/base/test/chrome/test_bug800386.xul

the problem is that xhr.channel.notificationCallbacks is set to the xhr.getInterface
but in nsXMLHttpRequest.cpp the xhr's mNotificationCallbacks is assigned to the channel's notificationCallbacks:

2875   // Hook us up to listen to redirects and the like
2876   mChannel->GetNotificationCallbacks(getter_AddRefs(mNotificationCallbacks));
2877   mChannel->SetNotificationCallbacks(this);

so there is an endless loop in OnProgress().

:bz : May I ask you to take a look at it? Please just look at the changes that I have made to test_bug800386.xul and tell me if that is enough for the test (I did not check bug800386 in details)
Attachment #8445782 - Flags: review?(bzbarsky)
this depends on previous patch.
Attachment #8445853 - Flags: review?(jduell.mcbugs)
Comment on attachment 8445782 [details] [diff] [review]
v1 - just check test_bug800386.xul

No, this totally breaks the test, as far as I can tell.  The test is written the way it is because Components.interfaces.nsIProgressEventSink is NOT the same sort of object as the argument to a JS-implemented getInterface when called from C++.  So prior to the fix for bug 800386 the former worked when passed to getInterface but the latter failed.

And easy way to check would be to back out the non-test parts of bug 800386 and see whether the test still fails....

Maybe the right thing to do here is something more like this:

-          return xhr.getInterface(aIID);
+          xhr.getInterface(aIID); /* Check that we can getInterface with aIID
+                                     without throwing */

That seems like it would keep testing what the test wants to test without causing loops in the notification callbacks.
Attachment #8445782 - Flags: review?(bzbarsky) → review-
> Maybe the right thing to do here is something more like this:
> 
> -          return xhr.getInterface(aIID);
> +          xhr.getInterface(aIID); /* Check that we can getInterface with
> aIID
> +                                     without throwing */
> 
> That seems like it would keep testing what the test wants to test without
> causing loops in the notification callbacks.

this works thanks,


actually, the test is doing a infinit look without this patch too, but it does not break the test:

0:11.85 ************************************************************
 0:11.85 * Call to xpconnect wrapped JSObject produced this error:  *
 0:11.85 [Exception... "[JavaScript Error: "too much recursion" {file: "chrome://mochitests/content/chrome/content/base/test/chrome/test_bug800386.xul" line: 38}]'[JavaScript Error: "too much recursion" {file: "chrome://mochitests/content/chrome/content/base/test/chrome/test_bug800386.xul" line: 38}]' when calling method: [nsIInterfaceRequestor::getInterface]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://mochitests/content/chrome/content/base/test/chrome/test_bug800386.xul :: requestor.getInterface :: line 38"  data: yes]
 0:11.85 ************************************************************
Attached patch Fix v2 (obsolete) — Splinter Review
Attachment #8445782 - Attachment is obsolete: true
Attachment #8445898 - Flags: review?(jduell.mcbugs)
Hmm, right you are.

So I think the right way to get rid of that recursion is to have two XHR objects, be registered as the callbacks on the channel of one of them but do our xhr.getInterface(aIID) on the other one.
Attached patch Fix v3 (obsolete) — Splinter Review
Attachment #8445898 - Attachment is obsolete: true
Attachment #8445898 - Flags: review?(jduell.mcbugs)
Attachment #8445912 - Flags: review?(bzbarsky)
Comment on attachment 8445912 [details] [diff] [review]
Fix v3

>+     * progress status notification is still elivered to the nsIProgressEventSink), 

"delivered".  And s/progress status/progress/ perhaps?

r=me on the non-HTTP bits; I assume someone else is reviewing the HTTP bits.
Attachment #8445912 - Flags: review?(bzbarsky) → review+
Comment on attachment 8445853 [details] [diff] [review]
bug614352 fix for nsXMLHttpRequest.cpp v1

Foisting this review on Steve.
Attachment #8445853 - Flags: review?(jduell.mcbugs) → review?(sworkman)
Comment on attachment 8445853 [details] [diff] [review]
bug614352 fix for nsXMLHttpRequest.cpp v1

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

Looks fine to me; just some renaming for clarification.

Is there adequate test coverage of compressed and uncompressed XHRs for these progress events?

::: content/base/src/nsXMLHttpRequest.cpp
@@ +778,5 @@
>  void
>  nsXMLHttpRequest::CreatePartialBlob()
>  {
>    if (mDOMFile) {
> +    if (mLoadTotal == mRealLoadTransferred) {

// Use progress info to determine if load is complete, but use mLoadTransferred to ensure a slice is created based on the uncompressed data count.

(s/mLoadTransferred/mDataReceived/ ... see elswhere in this review).

::: content/base/src/nsXMLHttpRequest.h
@@ +716,5 @@
>    uint64_t mLoadTotal; // 0 if not known.
> +  uint64_t mLoadTransferred;    // this is amount of recevied data it is the same as mRealLoadTransferred
> +                                // except for compressed data in this case mRealLoadTransferred is the amount
> +                                // of compressed data and mLoadTransferred the amount of uncompressed 
> +  uint64_t mRealLoadTransferred;

I know there is a comment at the end of a line for a var here, but let's just put the comments on a new line before the variables:

// Amount of uncompressed data received.
uint64_t mLoadTransferred;
// Amount of data received; compressed bytes if channel was compressed;
// otherwise this will equal mLoadTransferred.
uint64_t mRealLoadTransferred;


Also, I'm wondering if we can rename the vars to be clearer about the origin of their values:

-- mLoadTransferred is determined from OnDataAvailable, so let's change it to mDataReceived.
-- s/mReadLoadTransferred/mLoadProgress/ and s/mLoadTotal/mLoadProgressMax/ to bring them into line with OnProgress param names.
-- mLoadLengthComputable should be changed too. Go through the file and anywhere you see something like loadLength/mLoadLength, change it to hasLoadProgressMax/mHas~. Don't forget uploadLength too :)
Attachment #8445853 - Flags: review?(sworkman) → review+
Assignee: nobody → dd.mozilla
Comment on attachment 8445912 [details] [diff] [review]
Fix v3

Hi Steve,
You will need to review this one too :)
Attachment #8445912 - Flags: review?(sworkman)
Comment on attachment 8445912 [details] [diff] [review]
Fix v3

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

Looks good. Minor style/comment changes before you land the patch, please. r=me.

::: netwerk/base/public/nsIRequest.idl
@@ +124,2 @@
>       */
>      const unsigned long LOAD_BACKGROUND = 1 << 0; 

Since you're making changes here, please remove the end of line whitespace on lines 120 and 125 (and 122 in the comment).

Also, the comment is not 100% clear. I think it should be something like:

* Do not deliver status notifications to the nsIProgressEventSink and 
* do not block the loadgroup from completing (should this load belong to one).
* Note: Progress notifications will still be delivered.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +450,4 @@
>    {
>      // OnStatus
> +    // status is also blocked if channel has LOAD_BACKGROUND set.
> +    if (!(mLoadFlags & LOAD_BACKGROUND)) {

You can remove the extra comment; it's implicit in the if statement I think.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5443,2 @@
>          LOG(("sending status notification [this=%p status=%x progress=%llu/%llu]\n",
>              this, status, progress, progressMax));

Adjust this log so it takes LOAD_BACKGROUND into consideration. It would be nice to still see status and progress in the log, but also indicate if it's going to send progress only, or progress and status.

::: netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp
@@ +208,3 @@
>  
>    AutoEventEnqueuer ensureSerialDispatch(mEventQ);
>    

Please remove the extra whitespace on this line while you're making these changes.

@@ +217,2 @@
>      mProgressSink->OnProgress(this, nullptr, offset + data.Length(),
>                                uint64_t(mContentLength));

Since you're making a change here, please add braces for the if branch. Check other places in the patch too.

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +683,5 @@
>    LOG(("nsWyciwygChannel::OnDataAvailable [this=%p request=%x offset=%llu count=%u]\n",
>        this, request, offset, count));
>  
>    nsresult rv;
>    

Please remove the extra whitespace on this line while you're making these changes.
Attachment #8445912 - Flags: review?(sworkman) → review+
Attached patch fix v4 (obsolete) — Splinter Review
Attachment #8445912 - Attachment is obsolete: true
Attachment #8459482 - Flags: review?(sworkman)
(In reply to Steve Workman [:sworkman] from comment #17)
> Comment on attachment 8445853 [details] [diff] [review]
> bug614352 fix for nsXMLHttpRequest.cpp v1
> 
> Review of attachment 8445853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine to me; just some renaming for clarification.
> 
> Is there adequate test coverage of compressed and uncompressed XHRs for
> these progress events?


I wrote a test for this, just for me to be able to test it. But it involves connecting to a non-local address so I will not added it. Maybe XHR people know an existing test for this. I will ask, they should review this as well.


> ::: content/base/src/nsXMLHttpRequest.h
> @@ +716,5 @@
> >    uint64_t mLoadTotal; // 0 if not known.
> > +  uint64_t mLoadTransferred;    // this is amount of recevied data it is the same as mRealLoadTransferred
> > +                                // except for compressed data in this case mRealLoadTransferred is the amount
> > +                                // of compressed data and mLoadTransferred the amount of uncompressed 
> > +  uint64_t mRealLoadTransferred;
> 
> I know there is a comment at the end of a line for a var here, but let's
> just put the comments on a new line before the variables:
> 
> // Amount of uncompressed data received.
> uint64_t mLoadTransferred;
> // Amount of data received; compressed bytes if channel was compressed;
> // otherwise this will equal mLoadTransferred.
> uint64_t mRealLoadTransferred;
> 
> 
> Also, I'm wondering if we can rename the vars to be clearer about the origin
> of their values:
> 
> -- mLoadTransferred is determined from OnDataAvailable, so let's change it
> to mDataReceived.
> -- s/mReadLoadTransferred/mLoadProgress/ and s/mLoadTotal/mLoadProgressMax/
> to bring them into line with OnProgress param names.
> -- mLoadLengthComputable should be changed too. Go through the file and
> anywhere you see something like loadLength/mLoadLength, change it to
> hasLoadProgressMax/mHas~. Don't forget uploadLength too :)



I have changed names for:

s/mLoadTransferred/mDataAvailable/
s/mReadLoadTransferred/mLoadTransferred/

I do not want to change to much in nsXMLHttpRequest. because they have there own naming...
This is patch for XHR regarding progress events for compress data.
Attachment #8445853 - Attachment is obsolete: true
Attachment #8459562 - Flags: review?(bzbarsky)
(In reply to Steve Workman [:sworkman] from comment #17)

> Is there adequate test coverage of compressed and uncompressed XHRs for
> these progress events?

There's some coverage:
http://w3c-test.org/XMLHttpRequest/progress-events-response-data-gzip.htm - based on this bug, should pass after the fix

These tests should keep passing:
http://w3c-test.org/XMLHttpRequest/response-data-gzip.htm
http://w3c-test.org/XMLHttpRequest/response-data-deflate.htm

(response-data-gzip.htm has a bug right now, a fix is on the way)
(In reply to Hallvord R. M. Steen from comment #23)
> (In reply to Steve Workman [:sworkman] from comment #17)
> 
> > Is there adequate test coverage of compressed and uncompressed XHRs for
> > these progress events?
> 
> There's some coverage:
> http://w3c-test.org/XMLHttpRequest/progress-events-response-data-gzip.htm -
> based on this bug, should pass after the fix
> 
> These tests should keep passing:
> http://w3c-test.org/XMLHttpRequest/response-data-gzip.htm
> http://w3c-test.org/XMLHttpRequest/response-data-deflate.htm
> 
> (response-data-gzip.htm has a bug right now, a fix is on the way)

Thanks.

One question: 

OnProgress call changes the amount of transferred data, these are compressed, already transferred data (mLoadTransferred)
Data are collected after OnDataAvailable call and here mDataAvaialble is updated (mDataAvailable is the amount of decompressed data).

So the test relies on fact that mDataAvailabla and mLoadTransferred are updated at the same time, but they are not. I can make them to be updated at the same time but i think this is wrong, we should change the test

 if(e.lengthComputable && e.total && e.target.readyState < 4){
            assert_not_equals(e.total, e.loaded, 'total should not equal loaded while download/decode is incomplete')
          }
Flags: needinfo?(hsteen)
> So the test relies on fact that mDataAvailabla and mLoadTransferred are
> updated at the same time, but they are not. I can make them to be updated at
> the same time but i think this is wrong, we should change the test
> 
>  if(e.lengthComputable && e.total && e.target.readyState < 4){
>             assert_not_equals(e.total, e.loaded, 'total should not equal
> loaded while download/decode is incomplete')

Interesting. The logic here may well be faulty.

This assertation was intended to "catch" implementations that can't calculate "total" correctly but "cheat" by constantly updating it to match "loaded". The assumption is that while the content is still incoming, if the total length was computable, "total" should always be greater than "loaded" until the very end. The way the test is set up, the gzipped data from the request is meant to "trickle" in with some delays between chunks, thus we know the progress event will fire before everything is in and total should not equal loaded when it does.

I'm not going to argue against changing the test if you conclude the logic is wrong. It would however be nice if you could answer some questions so I understand this completely:

So e.loaded will reflect the amount of compressed data - per your comment we call this mLoadTransferred internally. I assume e.total will reflect the Content-Length header. Right? That's the only way you could use these values to calculate how many percentages of the complete content is downloaded. Where in this logic does mDataAvaialble values come in?
Flags: needinfo?(hsteen)
> Where in this logic does mDataAvaialble values come in?

I don't think it really does; I suspect the issue is the ordering of readyState transitions and progress events.

Consider a response that has "Content-Length: 100" and sends a 100 byte packet from the server.  Compression is not relevant here.

The network library will communicate that to us via a progress event for 100 bytes, then a "data available" event for 100 bytes, then a "response is done" event.

At which point in there do you expect the DOM "progress" event to fire?  At which point will "readyState" transition to 4?
Comment on attachment 8459562 [details] [diff] [review]
bug614352 fix for nsXMLHttpRequest.cpp v2

It worries me that we had no tests that needed changing.  We should add tests that would have failed without this patch but pass with it.

>+++ b/content/base/src/nsXMLHttpRequest.cpp
>+    // Use progress info to determine if load is complete, but use

s/if/whether/

>+++ b/content/base/src/nsXMLHttpRequest.h
>+  // Amount of uncompressed data received.
>+  uint64_t mDataAvailable;

Maybe replace "uncompressed" with "script-exposed (i.e. after undoing gzip compresion)"?

>+  // Amount of data received; compressed bytes if channel was compressed;

How about:

  Number of HTTP message body bytes received as of our last progress
  notification.  This quantity is in the same units as Content-Length and
  mLoadTotal, and hence counts compressed bytes when the channel has gzip
  Content-Encoding.  If the channel does not have Content-Encoding, this will
  be the same as mDataReceived except between the OnProgress that changes
  mLoadTransferred and the corresponding OnDataAvailable (which changes
  mDataReceived).

That's assuming that last claim is true.  Are we guaranteed OnProgress for some
new bytes will happen before the bytes are passed to OnDataAvailable?

r=me with those comment fixes an tests added.
Attachment #8459562 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #26)
> > Where in this logic does mDataAvaialble values come in?
> 
> I don't think it really does; I suspect the issue is the ordering of
> readyState transitions and progress events.
> 
> Consider a response that has "Content-Length: 100" and sends a 100 byte
> packet from the server.  Compression is not relevant here.
> 
> The network library will communicate that to us via a progress event for 100
> bytes, then a "data available" event for 100 bytes, then a "response is
> done" event.
> 
> At which point in there do you expect the DOM "progress" event to fire?  At
> which point will "readyState" transition to 4?

I also think that mDataAvailable is not relevant for progress events. But you can argue that data is receive only when XHR has it (on OnDataAvailable). I think we should consider data receive when transport sends OnProgress, so it can happen that all data is received but "readyState" is not 4.
 



> 
> That's assuming that last claim is true.  Are we guaranteed OnProgress for
> some
> new bytes will happen before the bytes are passed to OnDataAvailable?
> 


OnDataAvailable is called right after OnProgress (but we cannot guaranty that OnProgress will be called before OnDataAvailable, because OnProgress is dispatch to main the thread if we are not on the main thread)
(In reply to Boris Zbarsky [:bz] from comment #26)
> > Where in this logic does mDataAvaialble values come in?
> 
> I don't think it really does; I suspect the issue is the ordering of
> readyState transitions and progress events.
> 
> Consider a response that has "Content-Length: 100" and sends a 100 byte
> packet from the server.  Compression is not relevant here.

The intention in this test was that the backend sends three packets of data with a second delay - but it seems we get a single packet after three seconds? If I interpret Wireshark correctly.. In that case the test is currently buggy. I'll try a bigger size and make sure we have several packets on the wire.
> 
> r=me with those comment fixes an tests added.


is this test enough?
http://w3c-test.org/XMLHttpRequest/progress-events-response-data-gzip.htm -
(of course fixed)
Or we need another one?
> but we cannot guaranty that OnProgress will be called before OnDataAvailable

XHR is main-thread-only for now.  But maybe we should just document that the ordering of updates to the two quantities is undefined.

> The intention in this test was that the backend sends three packets of data with a second
> delay

That would have the same issue, no?  The key part is that HTTP libraries might not communicate "this is the last bit of data", so you end up with progress events at end of data before you know it's end of data.

> is this test enough?

Does it run on tbpl?  If not, then it's not enough.  The idea is to have a regression test for this in our continuous integration infrastructure.
I was wrong, a closer look at the Wireshark log shows that the 62 bytes of gzipped data (with HTTP headers it adds up to 222 bytes in total) are indeed split across four packets, sent with one second delays. (There's a Python backend for these tests to give us more control of HTTP-layer behaviour, so it should be consistent). 

However, the test is probably still asserting more than the spec really guarantees - I seem to remember that the spec used to have some text about fixing progress events every n milliseconds or after n bytes, whichever was more frequent, but I can't find this text now. That means we can not assert that an implementation should fire a "progress" event while loading those 62 bytes of data - even if it takes 3 seconds. It's up to the implementation. So I'll still change the test and give it more response data to load - to increase the likelyhood that we'll get a "progress" event before all the data is in.. The new test will have a payload of 165,375 bytes after gzipping.

Boris, does that sound good to you?
(I've also fixed the test to only do that assertation once - should avoid the "might run when all data is actually loaded but before readystatechange" race condition)
> Does it run on tbpl?

I would certainly hope that Mozilla runs the W3C's web-platform-tests suite continuously.. If we don't, that should be fixed!

(Especially since for a non-developer like me, getting tests added to the W3C suite is a lot easier than getting them into mozilla-central, so I tend to work on web-platform-tests :-p)
That seems pretty reasonable, yes.

> I would certainly hope that Mozilla runs the W3C's web-platform-tests suite continuously

Well, the patch makes no test changes, so either it's not passing tests before the patch, not passing after the patch, or we're not running them.

I'm pretty sure we're green before the patch, so either it would trigger failures on landing or we're not running the tests.  Given lack of try run, hard to tell, but given lack of a file named progress-events-response-data-gzip.htm in our tree, I'm pretty sure it's the latter.
Comment on attachment 8459482 [details] [diff] [review]
fix v4

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

Minor changes requested; r=me assuming they're made.

And it looks like a test is needed here from the recent comments :)

::: netwerk/base/public/nsIRequest.idl
@@ +124,1 @@
>       */

Whitespace at end of lines, including the '/**' line.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +451,5 @@
>    {
>      // OnStatus
> +    if (!(mLoadFlags & LOAD_BACKGROUND)) {
> +      MOZ_ASSERT(transportStatus == NS_NET_STATUS_RECEIVING_FROM ||
> +                 transportStatus == NS_NET_STATUS_READING);

I think these assertions are ok and useful outside the if block.

@@ +611,4 @@
>  
>    AutoEventEnqueuer ensureSerialDispatch(mEventQ);
>  
> +  // block socket status event after Cancel or OnStopRequest has been called

Block .... called.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5441,5 @@
>      // block socket status event after Cancel or OnStopRequest has been called.
> +    if (mProgressSink && NS_SUCCEEDED(mStatus) && mIsPending) {
> +        LOG(("sending progress%s notification [this=%p status=%x"
> +             " progress=%llu/%llu]\n",
> +            (mLoadFlags & LOAD_BACKGROUND)?"":" and status")

spaces: (mLoadFlags & LOAD_BACKGROUND)? "" : " and status")
Attachment #8459482 - Flags: review?(sworkman) → review+
Attached patch fix v5 (obsolete) — Splinter Review
Attachment #8459482 - Attachment is obsolete: true
Attachment #8460318 - Flags: review?(sworkman)
(In reply to Steve Workman [:sworkman] from comment #36)
> Comment on attachment 8459482 [details] [diff] [review]
> fix v4
> 
> Review of attachment 8459482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Minor changes requested; r=me assuming they're made.
> 
> And it looks like a test is needed here from the recent comments :)
> 
> ::: netwerk/base/public/nsIRequest.idl
> @@ +124,1 @@
> >       */
> 
> Whitespace at end of lines, including the '/**' line.
> 
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +451,5 @@
> >    {
> >      // OnStatus
> > +    if (!(mLoadFlags & LOAD_BACKGROUND)) {
> > +      MOZ_ASSERT(transportStatus == NS_NET_STATUS_RECEIVING_FROM ||
> > +                 transportStatus == NS_NET_STATUS_READING);
> 
> I think these assertions are ok and useful outside the if block.
> 

When I started working on this bu i tried to move this outside of if block, but it is crashing something is wrong with HttpChannelChild it is not connected to this bug
Attached patch Fix for bug 614352 v3 (obsolete) — Splinter Review
I added a test and change a comment a bit

instead of " Number of HTTP message body bytes received as of our last progress notification."

it is "Number of HTTP message body bytes received so far."
Attachment #8459562 - Attachment is obsolete: true
Attachment #8460330 - Flags: review?(bzbarsky)
Comment on attachment 8460330 [details] [diff] [review]
Fix for bug 614352 v3

This looks reasonable, but why do you need the huge test file?  Can you not use a small text file instead?  Something like a sequence of "a" characters should compress pretty nicely, for example.
Attachment #8460330 - Flags: review?(bzbarsky) → review+
Flags: needinfo?(dd.mozilla)
(In reply to Boris Zbarsky [:bz] from comment #40)
> Comment on attachment 8460330 [details] [diff] [review]
> Fix for bug 614352 v3
> 
> This looks reasonable, but why do you need the huge test file?  Can you not
> use a small text file instead?  Something like a sequence of "a" characters
> should compress pretty nicely, for example.

I just wanted to be sure that progress events are fired more than once. This one is probably too big the half would work as well, mss probably will not change...
Flags: needinfo?(dd.mozilla)
You could also just use an sjs to generate data on the fly instead of forcing everyone to download a big binary blob....
(In reply to Boris Zbarsky [:bz] from comment #42)
> You could also just use an sjs to generate data on the fly instead of
> forcing everyone to download a big binary blob....

I will change that
Comment on attachment 8460318 [details] [diff] [review]
fix v5

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

r=me - minor nits.

::: netwerk/base/public/nsIRequest.idl
@@ +117,5 @@
>       */
>      const unsigned long LOAD_NORMAL = 0;
>  
>      /** 
> +     * Do not deliver status notifications to the nsIProgressEventSink and 

Whitespace at end of these two lines!

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +442,5 @@
>    // Hold queue lock throughout all three calls, else we might process a later
>    // necko msg in between them.
>    AutoEventEnqueuer ensureSerialDispatch(mEventQ);
>  
> +  // block status/progress after Cancel or OnStopRequest has been called.

Oh I missed this one ;)  "Block..."

@@ +451,5 @@
>    {
>      // OnStatus
> +    if (!(mLoadFlags & LOAD_BACKGROUND)) {
> +      MOZ_ASSERT(transportStatus == NS_NET_STATUS_RECEIVING_FROM ||
> +                 transportStatus == NS_NET_STATUS_READING);

I'm fine with leaving these inside the if block, but can you look into why the crash happens if they are outside the block, please? - Follow-up item.
Attachment #8460318 - Flags: review?(sworkman) → review+
Attached patch Fix for bug 614352 v4 (obsolete) — Splinter Review
Attachment #8460330 - Attachment is obsolete: true
Attachment #8460767 - Flags: review?(bzbarsky)
Attached patch fix 6 (obsolete) — Splinter Review
Attachment #8460318 - Attachment is obsolete: true
Attachment #8460784 - Flags: review?(sworkman)
Attached patch Fix for bug 614352 v4 (obsolete) — Splinter Review
Attachment #8460767 - Attachment is obsolete: true
Attachment #8460767 - Flags: review?(bzbarsky)
Attachment #8460785 - Flags: review?(bzbarsky)
> 
> @@ +451,5 @@
> >    {
> >      // OnStatus
> > +    if (!(mLoadFlags & LOAD_BACKGROUND)) {
> > +      MOZ_ASSERT(transportStatus == NS_NET_STATUS_RECEIVING_FROM ||
> > +                 transportStatus == NS_NET_STATUS_READING);
> 
> I'm fine with leaving these inside the if block, but can you look into why
> the crash happens if they are outside the block, please? - Follow-up item.

I have investigated this, i have added some comments to be clearer what is happening.
Comment on attachment 8460785 [details] [diff] [review]
Fix for bug 614352 v4

Hmm.  Why a random string instead of a fixed string?  Seems like the latter would be better for the test being reliable.

Also, this is assuming that asyncConvertData finishes synchronously; it would be nice to avoid that (e.g. by having handleRequest call processAsync and then having the onStreamComplete do the response.write() itself and then call finish()).
(In reply to Boris Zbarsky [:bz] from comment #49)
> Comment on attachment 8460785 [details] [diff] [review]
> Fix for bug 614352 v4
> 
> Hmm.  Why a random string instead of a fixed string?  Seems like the latter
> would be better for the test being reliable.
> 

Having just 'a' compresses to good. I needed a bigger gzip data. Because the old code, at the end of a transferred, had put total=loaded; so if i want to check error in the old code i need more data at lease one progress event before a transfer ends.
I have change this in the next patch


> Also, this is assuming that asyncConvertData finishes synchronously; it
> would be nice to avoid that (e.g. by having handleRequest call processAsync
> and then having the onStreamComplete do the response.write() itself and then
> call finish()).

I am just very new to this all :)
Attached patch Fix for bug 614352 v5 (obsolete) — Splinter Review
Attachment #8460785 - Attachment is obsolete: true
Attachment #8460785 - Flags: review?(bzbarsky)
Attachment #8460904 - Flags: review?(bzbarsky)
Comment on attachment 8460904 [details] [diff] [review]
Fix for bug 614352 v5

You need to call response.processAsync(), no?  And response.finish(), not just finish()?

r=me with that.  Thanks!
Attachment #8460904 - Flags: review?(bzbarsky) → review+
Attachment #8460904 - Attachment is obsolete: true
Attachment #8461380 - Flags: review?(bzbarsky)
Comment on attachment 8460784 [details] [diff] [review]
fix 6

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

No need to ask for review again. r=me.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +447,3 @@
>    // or if channel has LOAD_BACKGROUND set.
> +  // If channel has LOAD_BACKGROUND set, OnProgress will be called in
> +  // RecvOnProgress.

// Note: Progress events will be received directly in RecvOnProgress if
// LOAD_BACKGROUND is set.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +708,5 @@
> +    // Send to child now. I've observed that this handles (i.e. non-ODA status
> +    // with progress > 0) is data upload progress notification
> +    // (status == NS_NET_STATUS_SENDING_TO).
> +    // This is also called if channel has LOAD_BACKGROUND set. OnStatus will
> +    // not be called to set mStoredStatus, but OnProgress will be called.

// Send OnProgress events to the child for data upload progress notifications
// (i.e. status == NS_NET_STATUS_SENDING_TO) or if the channel has
// LOAD_BACKGROUND set.
Attachment #8460784 - Flags: review?(sworkman) → review+
Attached patch fix 7Splinter Review
Attachment #8460784 - Attachment is obsolete: true
Attachment #8466066 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/572492b2e400
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.