1418545 - ASSERTION: count of zero passed to OnDataAvailable()

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Jason Duell]

:jesup reports that he's seeing 

[Parent 18513, Main Thread] ###!!! ASSERTION: count of zero passed to OnDataAvailable: 'Error', file /home/jesup/src/mozilla/inbound/netwerk/streamconv/converters/nsHTTPCompressConv.cpp, line 268

STR: Easily hittable by leaving a DEBUG build sitting on arstechnica.com or an article there.  Related to ads.

Comment 1

[Jason Duell]

:jesup: can you give us a stack trace? An NSPR log probably wouldn't hurt either.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

backtrace and poking around in ondata.log; http logs in the .gz files (I stopped the logging right after it hit).

https://app.box.com/s/7etjnni7yic8ty26rq9oi8vhzh4ncvcm

ASAN Debug+opt build, linux64, inbound rev 887fa3629f46

Comment 3

[Honza Bambas (:mayhemer)]

I think just adding |if (length) {}| around https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/netwerk/streamconv/converters/nsUnknownDecoder.cpp#867-872 would do.  It's legal IMO to don't have any data to convert.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Don't fire OnDataAvailable for 0 bytes

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8930364 [details] [diff] [review]
Don't fire OnDataAvailable for 0 bytes

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

::: netwerk/streamconv/converters/nsUnknownDecoder.cpp
@@ +863,5 @@
>          do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID);
>        if (!rawStream)
>          return NS_ERROR_FAILURE;
>  
> +      if (length) {

apprently you don't need to create the rawStream either, the four lines above should be moved to the conditioned block as well.

otherwise good.

thanks!

Comment 6

[Chris Peterson [:cpeterson]]

I hit these assertion failures when loading https://www.wired.com/ home page or articles in a debug build:

[Parent 14183, Main Thread] ###!!! ASSERTION: count of zero passed to OnDataAvailable: 'Error', file /Users/chris/Code/mozilla/firefox/netwerk/streamconv/converters/nsHTTPCompressConv.cpp, line 270
[Parent 14183, Main Thread] ###!!! ASSERTION: count of zero passed to OnDataAvailable: 'Error', file /Users/chris/Code/mozilla/firefox/netwerk/streamconv/converters/nsHTTPCompressConv.cpp, line 270
[Parent 14183, Main Thread] ###!!! ASSERTION: count of zero passed to OnDataAvailable: 'Error', file /Users/chris/Code/mozilla/firefox/netwerk/streamconv/converters/nsHTTPCompressConv.cpp, line 270

This bug has an r+'d patch that just needs some nits fixed before it can land.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7491e6c76312
Don't fire OnDataAvailable for 0 bytes r=mayhemer

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/7491e6c76312

Comment 9

[Chris Peterson [:cpeterson]]

Honza, if rawStream->SetData() or listener->OnDataAvailable() return an error after listener->OnStartRequest(), should this code call listener->OnStopRequest() with the rv error? The current implementation returns on error without calling OnStopRequest().

https://hg.mozilla.org/mozilla-central/rev/7491e6c76312#l1.35

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Chris Peterson [:cpeterson] from comment #9)
> Honza, if rawStream->SetData() or listener->OnDataAvailable() return an
> error after listener->OnStartRequest(), should this code call
> listener->OnStopRequest() with the rv error? The current implementation
> returns on error without calling OnStopRequest().
> 
> https://hg.mozilla.org/mozilla-central/rev/7491e6c76312#l1.35

The contract is to call OnStartRequest once also we then MUST call OnStopRequest, once.  Hence, yes, if OnData throws, we have to stop delivering data (don't call OnData any more) and call OnStop with the result of OnData:

rv = l->OnDataAvailable(..);
if (NS_FAILED(rv)) {
  l->OnStopRequest(request, rv);
  return ...;
}

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact which justifies backport consideration here or can this fix ride the trains?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

It's a debug-only issue, so I don't think it's important to uplift.