721627 - Don't return nsRefPtr/nsCOMPtr from nsDisplayImage

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

[Mats Palmgren (inactive)]

Comment on attachment 592037 [details] [diff] [review]
fix

r=mats

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/eb80ab6ee07b

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d68b6a9939d8

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This seems to have a caused crashes, e.g.
https://tbpl.mozilla.org/php/getParsedLog.php?id=8897664&tree=Mozilla-Inbound&full=1#error1
PROCESS-CRASH | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/bugs/641770-1.html | application crashed (minidump found)
 0  libxul.so!mozilla::layers::ImageLayer::ComputeEffectiveTransforms [ImageLayers.h : 355 + 0x1b]
    eip = 0x020e3a19   esp = 0xbfef62c0   ebp = 0xbfef6408   ebx = 0x02b339e8
    esi = 0x095a8adc   edi = 0x095a89d8   eax = 0x0afcfd00   ecx = 0xbfef6328
    edx = 0x00000000   efl = 0x00010206
    Found by: given as instruction pointer in context
 1  libxul.so!mozilla::layers::ContainerLayer::ComputeEffectiveTransformsForChildren [Layers.cpp : 489 + 0x8]
    eip = 0x020eb884   esp = 0xbfef6410   ebp = 0xbfef6428   ebx = 0x02b339e8
    esi = 0x095a89d8   edi = 0xbfef64e0
    Found by: call frame info

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like a dangling pointer...

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Aha, RasterImage (contrary to XPCOM rules) doesn't addref its out parameter.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Addref in RasterImage::GetImageContainer

Comment 9

[Joe Drew (not getting mail)]

Comment on attachment 592371 [details] [diff] [review]
fix v2

Review of attachment 592371 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/RasterImage.cpp
@@ +950,5 @@
>        (mImageContainer->Manager() == aManager || 
>         (!mImageContainer->Manager() && 
>          (mImageContainer->GetBackendType() == aManager->GetBackendType())))) {
>      *_retval = mImageContainer;
> +    NS_ADDREF(*_retval);

_yikes_. Who reviewed this originally anyways :)

::: layout/generic/nsImageFrame.cpp
@@ +1215,5 @@
>  {
> +  nsRefPtr<ImageContainer> container;
> +  nsresult rv = mImage->GetImageContainer(aManager, getter_AddRefs(container));
> +  NS_ENSURE_SUCCESS(rv, nsnull);
> +  return container.forget();

If you feel like it you could just return container.forget() here, since it will be NULL if it fails.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d9a6ddddfe

Comment 11

[Matt Brubeck (:mbrubeck)]

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