Closed Bug 1308289 Opened 3 years ago Closed 3 years ago

Crash in mozilla::image::VectorImage::OnSVGDocumentLoaded

Categories

(Core :: ImageLib, defect, P3)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Crash Signature: mozilla::image::VectorImage::OnSVGDocumentLoaded @0x0 | mozilla::image::VectorImage::OnSVGDocumentLoaded
Crash Signature: mozilla::image::VectorImage::OnSVGDocumentLoaded @0x0 | mozilla::image::VectorImage::OnSVGDocumentLoaded → [@ mozilla::image::VectorImage::OnSVGDocumentLoaded] [@ @0x0 | mozilla::image::VectorImage::OnSVGDocumentLoaded]
All of the reports consistently fail to call VectorImage::EvaluateAnimation, implemented by the base class ImageResource. Digging into the traces, it appears to be failing inside that call doing the VMT lookup for VectorImage::ShouldAnimate. The VMT pointer itself is 0xe5e5e5e5.

Every report I have looked at shows the same symptom (save the one variant linked above). Since it is consistently 0xe5e5e5e5, we can assume it is unlikely that the memory has been reallocated elsewhere between the free and the VMT access, and thus very little time has probably passed between those events.

SVGLoadEventListener is the object which receives the async event, and holds a raw pointer to a VectorImage. SVGLoadEventListener itself is owned by VectorImage and should be freed (and any events unsubscribed from) at the very latest when VectorImage is destroyed. The only other object which holds a reference to SVGLoadEventListener is the nsIDocument object, which it adds itself as an event listener to. We know that AddEventListener and RemoveEventListener are synchronous functions, so there should not be a reference to SVGLoadEventListener remaining after VectorImage.

We know from the ISupports signature for VectorImage that it is only accessed on the main thread so this is not a race condition on the free and the event coming in.

By process of elimination, the VectorImage is likely getting freed on the SVGLoadEventListener event handling itself. SVGLoadEventListener::HandleEvent retains a strong reference to itself before triggering the event because it will get freed before it completes, but it does not upgrade its raw pointer to VectorImage to a strong reference. ProgressTracker::SyncNotifyProgress may triggered in VectorImage::OnSVGDocumentLoaded (and OnSVGDocumentError for that matter) which can go to many different places in the layout/CSS/DOM code which hold references to VectorImage. Should their reference get cleared here, it is possible VectorImage gets freed midway through handling the event. Note that the next we do after ProgressTracker::SyncNotify is VectorImage::EvaluateAnimation, the place where we crash.

So the sequence of events is as follows:

1) A listener to ProgressTracker's events (directly or indirectly) holds the last strong reference to VectorImage.
2) The MozSVGAsImageDocumentLoad event is triggered by the document.
3) SVGLoadEventListener::HandleEvent is called.
4) VectorImage::OnSVGDocumentLoaded is called.
5) ProgressTracker::SyncNotifyProgress is called, and the listener releases the last reference to VectorImage.
6) VectorImage is destroyed and its memory set to 0xe5.
7) We return from ProgressTracker::SyncNotifyProgress.
8) VectorImage::EvaluateAnimation is called and the VMT lookup fails.

If this is indeed the problem, all we should need to do is hold a temporary strong reference to VectorImage in our event handlers.
Note I have not been able to reproduce this locally and it only seems to happen on Android builds...

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=735c20936238e274495dafd6df31b46d282be10c
Attachment #8798561 - Flags: review?(tnikkel)
Has STR: --- → no
Keywords: crash
Priority: -- → P3
Comment on attachment 8798561 [details] [diff] [review]
Hold strong reference to VectorImage when processing events, v1

I think I would prefer the strong references to the VectorImage to be held inside the function where it can be destroyed, ie OnSVGDocumentLoaded. And we should probably note which function can destroy this in comments.

There are other callers of SyncNotifyProgress in VectorImage, so don't they need the same treatment?
Attachment #8798561 - Flags: review?(tnikkel)
(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> Comment on attachment 8798561 [details] [diff] [review]
> Hold strong reference to VectorImage when processing events, v1
> 
> I think I would prefer the strong references to the VectorImage to be held
> inside the function where it can be destroyed, ie OnSVGDocumentLoaded. And
> we should probably note which function can destroy this in comments.
> 

Done.

> There are other callers of SyncNotifyProgress in VectorImage, so don't they
> need the same treatment?

Good question. Crashes have only been observed in OnSVGDocumentLoaded. In general SyncNotifyProgress is the last thing we do, so even if the VectorImage got freed, it did not need to read/write the state. However there are two other places we set the state after SyncNotifyProgress and I've updated those accordingly.
Attachment #8798561 - Attachment is obsolete: true
Attachment #8800618 - Flags: review?(tnikkel)
Attachment #8800618 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/956f4eff32d2
Hold strong reference to VectorImage when dispatching ProgressTracker::SyncNotifyProgress. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/956f4eff32d2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Crash Signature: [@ mozilla::image::VectorImage::OnSVGDocumentLoaded] [@ @0x0 | mozilla::image::VectorImage::OnSVGDocumentLoaded] → [@ mozilla::image::VectorImage::OnSVGDocumentLoaded] [@ @0x0 | mozilla::image::VectorImage::OnSVGDocumentLoaded] [@ mozilla::image::ImageResource::EvaluateAnimation ]
Comment on attachment 8800618 [details] [diff] [review]
Hold strong reference to VectorImage when processing events, v2

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: May crash when opening an SVG.
[Describe test coverage new/current, TreeHerder]: No STR but observable through crash reports. Existing mochitests exercise the added code.
[Risks and why]: Very small. We just add an extra temporary strong reference on the stack to the SVG image. It should have no impact besides briefly extending the life of the object (and none if there are other strong references).
[String/UUID change made/needed]: N/A
Attachment #8800618 - Flags: approval-mozilla-aurora?
Comment on attachment 8800618 [details] [diff] [review]
Hold strong reference to VectorImage when processing events, v2

Fix a crash. Take it in 51 aurora.
Attachment #8800618 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.