Closed Bug 1418545 Opened 2 years ago Closed Last year

ASSERTION: count of zero passed to OnDataAvailable()

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
geckoview62 --- wontfix
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: jesup)

Details

(Keywords: assertion, Whiteboard: [necko-triaged])

Attachments

(1 file)

: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.
:jesup: can you give us a stack trace? An NSPR log probably wouldn't hurt either.
Flags: needinfo?(rjesup)
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
Flags: needinfo?(rjesup)
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.
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
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!
Attachment #8930364 - Flags: review?(honzab.moz) → review+
Priority: -- → P3
Whiteboard: [necko-triaged]
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.
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7491e6c76312
Don't fire OnDataAvailable for 0 bytes r=mayhemer
https://hg.mozilla.org/mozilla-central/rev/7491e6c76312
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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
Flags: needinfo?(honzab.moz)
(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 ...;
}
Flags: needinfo?(honzab.moz)
Is there a user impact which justifies backport consideration here or can this fix ride the trains?
Flags: needinfo?(rjesup)
It's a debug-only issue, so I don't think it's important to uplift.
Flags: needinfo?(rjesup)
You need to log in before you can comment on or make changes to this bug.