The default bug view has changed. See this FAQ.

Don't return nsRefPtr/nsCOMPtr from nsDisplayImage

RESOLVED FIXED in mozilla12

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

Trunk
mozilla12
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
Created attachment 592037 [details] [diff] [review]
fix
Attachment #592037 - Flags: review?(matspal)
Comment on attachment 592037 [details] [diff] [review]
fix

r=mats
Attachment #592037 - Flags: review?(matspal) → review+
Attachment #592037 - Flags: checkin?
http://hg.mozilla.org/integration/mozilla-inbound/rev/eb80ab6ee07b
Attachment #592037 - Flags: checkin?
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d68b6a9939d8
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
Looks like a dangling pointer...
Aha, RasterImage (contrary to XPCOM rules) doesn't addref its out parameter.
Created attachment 592371 [details] [diff] [review]
fix v2

Addref in RasterImage::GetImageContainer
Attachment #592371 - Flags: review?(joe)
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.
Attachment #592371 - Flags: review?(joe) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d9a6ddddfe
https://hg.mozilla.org/mozilla-central/rev/f7d9a6ddddfe
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.