473933 - Crash in necko!nsCOMPtr<nsIUnicharStreamLoaderObserver>::assign_assuming_AddRef

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[[e-mail]]

If you don't have a safe browsing service then Gecko crashes double-releasing a SheetLoadData object. In fact the code was wrong before, but the safe browsing check simply adds a much more visible failure mode.

CSSLoaderImpl::LoadStyleLink creates a SheetLoadData and manually NS_ADDREFs it. However if LoadSheet fails, in this case through a lack of a safe browsing service, it calls SheetComplete which manually NS_RELEASEs it. Cancelling the channel also calls SheetComplete which NS_RELEASEs it again. The pointer is now stale at this point, although the channel still thinks it owns a reference.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Remove both the original bogus SheetComplete call and the copied one.

Comment 2

[Dave Camp (:dcamp)]

Attachment: don't fail on missing service

While that patch is necessary, there's another problem here;  Both pre-existing classifier callbacks (docshell and nsObjectLoadingContent) ignored the error when the classifier wasn't available.  Attached bug moves that ignored error into the classifier object itself so we don't need that logic everywhere.

Comment 3

[Dave Camp (:dcamp)]

(In reply to comment #2)
> While that patch is necessary, there's another problem here;

Another problem, deserving of another bug, which had already been filed.  Patch moved there.

Comment 4

[[e-mail]]

Pushed changeset 5e601b6daab1 to mozilla-central.

Comment 5

[[e-mail]]

Comment on attachment 357340 [details] [diff] [review]
Proposed patch

Fix crash when stylesheet is blocked by safe browsing (or lack of, until bug 473938 is fixed).

Comment 8

[Frank Wein [:mcsmurf]]

Checked the patch into 1.9.1 branch to get the tests and SeaMonkey builds running again, changeset 3038fdb65bf9.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Uh... the existing SheetComplete() call was not bogus.  What made you think it was, exactly?

It probably doesn't matter that much, since ideally this code will go away once we move to no-fail allocation, but the problem was that the code was copied incorrectly, not that the existing code was wrong.

It'd be awful nice if I got cced on changes to the cssloader, for what it's worth.

Comment 10

[[e-mail]]

Attachment: Oops

Sorry, I blamed it because it was being called on a deleted object. What I failed to notice was that there was a way to avoid it getting deleted.

Comment 11

[[e-mail]]

Comment on attachment 358843 [details] [diff] [review]
Oops

Given that this is a necessary followup to attachment 357340 [details] [diff] [review]...

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 358843 [details] [diff] [review]
Oops

a191=beltzner

Comment 13

[[e-mail]]

Pushed changeset 93bf83df396b to mozilla-central.

Pushed changeset 9f49e339c435 to mozilla-1.9.1