Closed Bug 473933 Opened 11 years ago Closed 11 years ago

Crash in necko!nsCOMPtr<nsIUnicharStreamLoaderObserver>::assign_assuming_AddRef

Categories

(Toolkit :: Safe Browsing, defect, P2, blocker)

3.5 Branch
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

(Keywords: crash, fixed1.9.1, regression)

Attachments

(3 files)

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.
Flags: blocking-firefox3.1?
Attached patch Proposed patchSplinter Review
Remove both the original bogus SheetComplete call and the copied one.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #357340 - Flags: superreview?(jonas)
Attachment #357340 - Flags: review?(jonas)
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.
Attachment #357359 - Flags: superreview?(jonas)
Attachment #357359 - Flags: review?(jonas)
(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.
Attachment #357359 - Flags: superreview?(jonas)
Attachment #357359 - Flags: review?(jonas)
Attachment #357340 - Flags: superreview?(jonas)
Attachment #357340 - Flags: superreview+
Attachment #357340 - Flags: review?(jonas)
Attachment #357340 - Flags: review+
Pushed changeset 5e601b6daab1 to mozilla-central.
Severity: major → blocker
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Version: 3.0 Branch → 3.1 Branch
Attachment #357340 - Flags: approval1.9.1?
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).
Duplicate of this bug: 474101
Duplicate of this bug: 474101
Attachment #357340 - Flags: approval1.9.1? → approval1.9.1+
Checked the patch into 1.9.1 branch to get the tests and SeaMonkey builds running again, changeset 3038fdb65bf9.
Keywords: fixed1.9.1
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.
Attached patch OopsSplinter Review
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.
Attachment #358843 - Flags: superreview?(bzbarsky)
Attachment #358843 - Flags: review?(bzbarsky)
Attachment #358843 - Flags: superreview?(bzbarsky)
Attachment #358843 - Flags: superreview+
Attachment #358843 - Flags: review?(bzbarsky)
Attachment #358843 - Flags: review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 358843 [details] [diff] [review]
Oops

Given that this is a necessary followup to attachment 357340 [details] [diff] [review]...
Attachment #358843 - Flags: approval1.9.1?
Comment on attachment 358843 [details] [diff] [review]
Oops

a191=beltzner
Attachment #358843 - Flags: approval1.9.1? → approval1.9.1+
Flags: blocking-firefox3.1? → blocking-firefox3.1+
Priority: -- → P2
Pushed changeset 93bf83df396b to mozilla-central.

Pushed changeset 9f49e339c435 to mozilla-1.9.1
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.