1275811 - Crash in free_impl | nsACString_internal::Replace | mozilla::places::AsyncFetchAndSetIconForPage

Status: RESOLVED WONTFIX

(Toolkit :: Places, defect, P2)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug was filed from the Socorro interface and is 
report bp-7dfd9bb0-5e3a-4496-ae6d-8a2262160524.
=============================================================

Comment 1

[Andrew McCreight [:mccr8]]

Maybe this is related to bug 1255270? Can you take a look, Gijs?

Comment 2

[:Gijs (he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Maybe this is related to bug 1255270? Can you take a look, Gijs?

I'm not entirely sure... It's been uplifted, and the only crashes are on Nightly. No STR. Not sure what's happening. The only thing I *think* might be happening is that somehow onDataAvailable is still called after we've already canceled the request, and mIcon is somehow null, or something? There is no guard for mCanceled in OnDataAvailable - maybe there should be? But I'm not at all sure I'm reading the stack correctly. Marco, thoughts?

Comment 3

[Marco Bonardo [:mak]]

The first thing I wonder about, is whether it's safe to use buffer when we get NS_BASE_STREAM_WOULD_BLOCK. In general, the output params should never be used in case of an error. But I don't know how one should handle an NS_BASE_STREAM_WOULD_BLOCK from NS_ConsumeStream, the documentation says:
"This allows one to use this function with a non-blocking input stream that may return NS_BASE_STREAM_WOULD_BLOCK if it only has partial data available."
that doesn't tell me if it's handling it internally or one is still supposed to handle that manually.
Off-hand I think we should just change that to if (NS_FAILED(rv)) return rv;

I don't think mIcon can be nullified, and even if we abort and OnDataAvailable is still invoked, I don't see why we should crash on a string memory operation :/
nsIRequest states:
"Requests that use nsIStreamListener must not call onDataAvailable anymore after cancel has been called.", so unless we have a bogus request, it should not even be invoked. (a MOZ_ASSERT(!mCanceled) would be nice though!).

So far, apart from the possibily wrong handling of the return value, I don't see other causes. The fact is that if that would be our problem, then I'd expect any version to be affected, not just Nightly.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.