1335321 - stylo: Unsafe parallel refcount drop in CachedBorderImageData::PurgeCachedImages

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

> > Error: AddRef/Release on nsISupports
> > Location: nsCOMArray.cpp:void ReleaseObjects(class nsTArray<nsISupports*>*)
> > @ xpcom/glue/nsCOMArray.cpp:272
> > Stack Trace:
> > void nsCOMArray_base::Clear() @ xpcom/glue/nsCOMArray.cpp:281
> > void CachedBorderImageData::PurgeCachedImages() @
> > layout/style/nsStyleStruct.cpp:2044
> > void nsStyleImage::SetImageRequest(struct
> > already_AddRefed<nsStyleImageRequest>) @ layout/style/nsStyleStruct.cpp:2150
> > Gecko_SetUrlImageValue @ layout/style/ServoBindings.cpp:752

Comment 1

[Bobby Holley (:bholley)]

Manish, you were working on this code, right? WDYT?

Comment 2

[Manish Goregaokar [:manishearth]]

I think heycam will have a better idea -- he did the final implementation of the url image stuff.

Comment 3

[Bobby Holley (:bholley)]

Some naive options:
* Check IsInServoTraversal in PurgeCachedImages, and NS_ReleaseOnMainThread if so.
* Queue up this work (on the servo side) in SequentialTasks to execute post-traversal.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1335321 - stylo: Do CachedBorderImageData::PurgeCachedImages work on the main thread.

Review commit: https://reviewboard.mozilla.org/r/113864/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113864/

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8839141 [details]
Bug 1335321 - stylo: Do CachedBorderImageData::PurgeCachedImages work on the main thread.

https://reviewboard.mozilla.org/r/113864/#review115456

::: layout/style/nsStyleStruct.cpp:2074
(Diff revision 1)
>  CachedBorderImageData::PurgeCachedImages()
>  {
> +  if (ServoStyleSet::IsInServoTraversal()) {
> +    RefPtr<PurgeCachedImagesTask> task = new PurgeCachedImagesTask();
> +    task->mSubImages.SwapElements(mSubImages);
> +    NS_DispatchToMainThread(task.forget());

Maybe add a comment saying that this won't proxy if we're already on the main thread, and that's fine.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8839141 [details]
Bug 1335321 - stylo: Do CachedBorderImageData::PurgeCachedImages work on the main thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113864/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b393d50b5ed8
stylo: Do CachedBorderImageData::PurgeCachedImages work on the main thread. r=bholley

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b393d50b5ed8