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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

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..

Comment 1

[Jason Duell]

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.

Comment 2

[Dragana Damjanovic [:dragana]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 4

[Jason Duell]

> 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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Jason Duell]

> 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

Comment 7

[Dragana Damjanovic [:dragana]]

about chunked transfer-encoding:
progressMax is set to UINT64_MAX and progress is update the same as with no encoding.

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: v1 - just check test_bug800386.xul

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)

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug614352 fix for nsXMLHttpRequest.cpp v1

this depends on previous patch.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Dragana Damjanovic [:dragana]]

> 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 ************************************************************

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: Fix v2

Comment 13

[Boris Zbarsky [:bzbarsky]]

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.

Comment 14

[Dragana Damjanovic [:dragana]]

Attachment: Fix v3

Comment 15

[Boris Zbarsky [: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.

Comment 16

[Jason Duell]

Comment on attachment 8445853 [details] [diff] [review]
bug614352 fix for nsXMLHttpRequest.cpp v1

Foisting this review on Steve.

Comment 17

[Steve Workman [:sworkman] (INACTIVE)]

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 :)

Comment 18

[Dragana Damjanovic [:dragana]]

Comment on attachment 8445912 [details] [diff] [review]
Fix v3

Hi Steve,
You will need to review this one too :)

Comment 19

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 20

[Dragana Damjanovic [:dragana]]

Attachment: fix v4

Comment 21

[Dragana Damjanovic [:dragana]]

(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...

Comment 22

[Dragana Damjanovic [:dragana]]

Attachment: bug614352 fix for nsXMLHttpRequest.cpp v2

This is patch for XHR regarding progress events for compress data.

Comment 23

[Hallvord R. M. Steen [:hallvors]]

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

Comment 24

[Dragana Damjanovic [:dragana]]

(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')
          }

Comment 25

[Hallvord R. M. Steen [:hallvors]]

> 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?

Comment 26

[Boris Zbarsky [:bzbarsky]]

> 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 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Dragana Damjanovic [:dragana]]

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

Comment 29

[Hallvord R. M. Steen [:hallvors]]

(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.

Comment 30

[Dragana Damjanovic [:dragana]]

> 
> 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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 32

[Hallvord R. M. Steen [:hallvors]]

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?

Comment 33

[Hallvord R. M. Steen [:hallvors]]

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

Comment 34

[Hallvord R. M. Steen [:hallvors]]

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

Comment 35

[Boris Zbarsky [:bzbarsky]]

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 36

[Steve Workman [:sworkman] (INACTIVE)]

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")

Comment 37

[Dragana Damjanovic [:dragana]]

Attachment: fix v5

Comment 38

[Dragana Damjanovic [:dragana]]

(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

Comment 39

[Dragana Damjanovic [:dragana]]

Attachment: Fix for bug 614352 v3

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."

Comment 40

[Boris Zbarsky [: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.

Comment 41

[Dragana Damjanovic [:dragana]]

(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...

Comment 42

[Boris Zbarsky [:bzbarsky]]

You could also just use an sjs to generate data on the fly instead of forcing everyone to download a big binary blob....

Comment 43

[Dragana Damjanovic [:dragana]]

(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 44

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 45

[Dragana Damjanovic [:dragana]]

Attachment: Fix for bug 614352 v4

Comment 46

[Dragana Damjanovic [:dragana]]

Attachment: fix 6

Comment 47

[Dragana Damjanovic [:dragana]]

Attachment: Fix for bug 614352 v4

Comment 48

[Dragana Damjanovic [:dragana]]

> 
> @@ +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 49

[Boris Zbarsky [:bzbarsky]]

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

Comment 50

[Dragana Damjanovic [:dragana]]

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

Comment 51

[Dragana Damjanovic [:dragana]]

Attachment: Fix for bug 614352 v5

Comment 52

[Boris Zbarsky [: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!

Comment 53

[Dragana Damjanovic [:dragana]]

Attachment: Fix for bug 614352 v6

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8461380 [details] [diff] [review]
Fix for bug 614352 v6

r=me

Comment 55

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 56

[Dragana Damjanovic [:dragana]]

Attachment: fix 7

Comment 57

[Dragana Damjanovic [:dragana]]

https://tbpl.mozilla.org/?tree=Try&rev=1e083090c284

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/572492b2e400

I landed the other patch under bug 614352.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/572492b2e400