Last Comment Bug 721627 - Don't return nsRefPtr/nsCOMPtr from nsDisplayImage
: Don't return nsRefPtr/nsCOMPtr from nsDisplayImage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-26 20:07 PST by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-01-30 19:11 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (3.90 KB, patch)
2012-01-26 20:08 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
Details | Diff | Review
fix v2 (5.21 KB, patch)
2012-01-28 00:12 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
joe: review+
Details | Diff | Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-26 20:07:59 PST

    
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-26 20:08:48 PST
Created attachment 592037 [details] [diff] [review]
fix
Comment 2 Mats Palmgren (:mats) 2012-01-27 01:36:45 PST
Comment on attachment 592037 [details] [diff] [review]
fix

r=mats
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 19:40:35 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/eb80ab6ee07b
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 20:49:54 PST
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/d68b6a9939d8
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 20:54:36 PST
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) (Exited; email my personal email if necessary) 2012-01-27 21:07:54 PST
Looks like a dangling pointer...
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-27 21:10:27 PST
Aha, RasterImage (contrary to XPCOM rules) doesn't addref its out parameter.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-28 00:12:28 PST
Created attachment 592371 [details] [diff] [review]
fix v2

Addref in RasterImage::GetImageContainer
Comment 9 Joe Drew (not getting mail) 2012-01-28 12:54:21 PST
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) (Exited; email my personal email if necessary) 2012-01-29 23:54:35 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d9a6ddddfe
Comment 11 Matt Brubeck (:mbrubeck) 2012-01-30 19:11:40 PST
https://hg.mozilla.org/mozilla-central/rev/f7d9a6ddddfe

Note You need to log in before you can comment on or make changes to this bug.