587902 - Don't instantiate imgRequest::mImage until *after* we know mimetype

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

The changeset below (Bug 572520 step 6)...
http://hg.mozilla.org/mozilla-central/diff/17efdfae5c82/modules/libpr0n/src/imgRequest.cpp
... made us instantiate imgRequest::mImage earlier than we used to.  We used to do it in OnDataAvailable, but now we do it inside of imgRequest::Init.

However, for SVG-as-image to work, we can't do that early.  We need to wait until we know the mimetype (in OnDataAvailable), because that tells us the correct type for mImage (RasterImage vs. VectorImage).

Filing this bug on deferring the instantiation of imgRequest::mImage so that this can work.

Joe suggests in IRC that we still instantiate an imgStatusTracker up-front, and then link it up with its Image later on. (once we've instantiated mImage)

Comment 1

[Joe Drew (not getting mail)]

Unfortunately, this means the Image no longer holds on to its status.

Comment 2

[Daniel Holbert [:dholbert]]

Can't we...
 (a) remove the Image pointer from the imgStatusTracker constructor
 (b) add an imgStatusTracker pointer to the Image constructor?

This way the imgStatusTracker would live on the imgRequest, but Image would have a (raw) pointer to it.

This is safe as long as
 - an Image is always owned by the same imgRequest
 - an imgRequest will only ever own at most one Image, in its lifetime

I think those constraints are both satisfied, so this should be ok, right?

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
>  - an Image is always owned by the same imgRequest
>  - an imgRequest will only ever own at most one Image, in its lifetime
> 
> I think those constraints are both satisfied, so this should be ok, right?

D'oh -- So, the first constraint there isn't quite true, because not all Images are imgRequest-owned.  In particular, RasterImage::ExtractFrame creates an Image that's not owned by an imgReqeuest at all.

So if imgRequest owns the status tracker (and hands its mImage a raw pointer to it), then ExtractFrame-generated Images wouldn't get a status tracker.

Comment 4

[Daniel Holbert [:dholbert]]

Just talked this through in #gfx -- so I think all will be well if Image and imgRequest both have a nsAutoPtr<imgStatusTracker>, which gets transferred from imgRequest --> Image when an imgRequest's Image gets instantiated.  (and which unowned Images -- e.g. from ExtractFrame -- can populate on their own)

With some hand-waving, I think that should work...

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Part 1: Allow imgStatusTracker to be created before its Image

This patch simply lets us create an imgStatusTracker *before* we create its Image.  Then, when we create the Image, we pass in a pointer to the imgStatusTracker in the Image's constructor, and the Image takes ownership of it (and informs the tracker about its own address).

The no-argument Image constructor still works as before -- instantiating its own imgStatusTracker.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Part 2: Construct Image later, and use imgRequest::mStatusTracker before we've constructed it

This patch makes this bug's most important change -- constructing imgRequest::mImage **after** we know its mimetype.  At that point, we transfer the imgStatusTracker to the newly-constructed Image, and we notify all our imgRequestProxy observers about our Image.

Some important invariants that are maintained by this patch, to keep in mind:
 - An imgRequest will *always* have an imgStatusTracker (after Init-time) -- but once it's created its mImage, the status tracker will be stored there.
 - An Image will also *always* have an imgStatusTracker (after construction-time).

NOTE: This patch also removes the method "Image::IsInitialized", which is no longer needed. (its callers have been replaced with null-checks)

Comment 7

[Daniel Holbert [:dholbert]]

Requesting blocking status, as this is needed for blocker-bug 276431.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Part 2: Construct Image later, and use imgRequest::mStatusTracker before we've constructed it

Sorry -- the first version of Part 2 was based on top of a patch from bug 276431.  I just reordered patch queue & noticed that & fixed it in this version.  (doesn't really affect anything -- just contextual code, basically)

FWIW, in my patch queue, this bug's patches are based on top of the patches for bug 587371, bug 587779, and bug 587800.  (I intend for this to land after those bugs do.)

Comment 9

[(no longer active)]

There is a chance that this bug might fix the leak I uncovered with the test in bug 586436.  Adding that bug as a dependency so that I look at this again when this is resolved.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Part 2 v2: Construct Image later, and use imgRequest::mStatusTracker before we've constructed it

New version of part 2, with a one-liner-fix to a bug uncovered on TryServer XPCShell tests. (In imgRequestProxy::Clone, we need to support giving the clone *either* mImage, if non-null, or mOwner->mImage.  We can't unconditionally pass mImage, because it's possible mImage is null and mOwner->mImage is non-null -- if we got canceled before mOwner instantiated its image.  And we can't unconditionally pass mOwner->mImage, because we might not have a mOwner in the first place.)

Also, RE leaks -- it looks like this actually _causes_ a leak in debug builds right now, according to tryserver. :)  I'm investigating that... (leaving review request open, as I'm not expecting the leak-fix to be a significant change to the patch)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #10)
> Also, RE leaks -- it looks like this actually _causes_ a leak in debug builds
(FWIW, this leak went away on the next TryServer run, so I'm not convined it's real)

Comment 12

[Daniel Holbert [:dholbert]]

Yup, the leak from Comment 10 away on subsequent TryServer runs, so I think it was a fluke.

There's one remaining issue on TryServer -- an abort-if-false failure in/around the crashtest 467703-1.xhtml. I can't reproduce it locally, so I've pushed some debugging code to try, to hopefully figure out what's going on...

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Part 2 v3: Construct Image later, and use imgRequest::mStatusTracker before we've constructed it

Addressed comment 12 in this version -- just needed to handle the case where an imgRequestProxy gets canceled, and then its imgRequest instantiates its image after that, and then our imgRequestProxy receives a call to GetImage. (See coment inside of imgRequestProxy::GetImage for more detail)

Latest tryserver build, with this version, is all green (discounting 2 known-sporadic testfailures and the perma-orange "RGL" boxes).

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: Part 3 ( assertions): Add some NS_ABORT_IF_FALSE for assumptions in imgStatusTracker methods

Comment 15

[Joe Drew (not getting mail)]

Required for <img src=svg>

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 467144 [details] [diff] [review]
Part 1: Allow imgStatusTracker to be created before its Image

>+void
>+imgStatusTracker::SetImage(Image* aImage)
>+{
>+  NS_ABORT_IF_FALSE(!mImage, "Setting null image");

I think this was supposed to be aImage here.

>+  NS_ABORT_IF_FALSE(!mImage, "Setting image when we already have one");
>+  mImage = aImage;
>+}

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 467544 [details] [diff] [review]
Part 3 ( assertions): Add some NS_ABORT_IF_FALSE for assumptions in imgStatusTracker methods

hooray

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 467543 [details] [diff] [review]
Part 2 v3: Construct Image later, and use imgRequest::mStatusTracker before we've constructed it


>+imgStatusTracker&
>+imgRequest::GetStatusTracker()
>+{
>+  NS_ABORT_IF_FALSE((mStatusTracker || mImage) && (!mStatusTracker || !mImage),
>+                    "exactly one of [mStatusTracker, mImage] "
>+                    "should be non-null");
>+  return mImage ? mImage->GetStatusTracker() : *mStatusTracker;

This seems a little complex. How about:

if (mImage)
  NS_ABORT_IF_FALSE(!mStatusTracker)
  return mImage->GetStatusTracker()
else
  NS_ABORT_IF_FALSE(mStatusTracker)
  NS_ABORT_IF_FALSE(!mImage)
  return *mStatusTracker

>   // Tell the image that it has all of the source data. Note that this can
>   // trigger a failure, since the image might be waiting for more non-optional
>   // data and this is the point where we break the news that it's not coming.
>-  if (mImage->IsInitialized() &&
>+  if (mImage &&
>       mImage->GetType() == imgIContainer::TYPE_RASTER) {

I think this'll fit on a single line, won't it?


> /* attribute imgIContainer image; */
> NS_IMETHODIMP imgRequestProxy::GetImage(imgIContainer * *aImage)
> {
>-  if (!mImage->IsInitialized())
>+  // It's possible that our owner has an image but hasn't notified us of it -
>+  // that'll happen if we get Canceled before the owner instantiates its image
>+  // (because Canceling unregisters us as a listener on mOwner).  // If we're
>+  // in that situation, just use grab the image off of mOwner.

"use grab"


>@@ -734,25 +767,57 @@ void imgRequestProxy::NotifyListener()
> {
>   // It would be nice to notify the observer directly in the status tracker
>   // instead of through the proxy, but there are several places we do extra
>   // processing when we receive notifications (like OnStopRequest()), and we
>   // need to check mCanceled everywhere too.
> 
>   if (mOwner) {
>     // Send the notifications to our listener asynchronously.
>-    mImage->GetStatusTracker().Notify(mOwner, this);
>+    imgStatusTracker& statusTracker = GetStatusTracker();
>+    statusTracker.Notify(mOwner, this);

Why not just GetStatusTracker().Notify()?

>diff --git a/modules/libpr0n/src/imgStatusTracker.cpp b/modules/libpr0n/src/imgStatusTracker.cpp
>--- a/modules/libpr0n/src/imgStatusTracker.cpp
>+++ b/modules/libpr0n/src/imgStatusTracker.cpp
>@@ -105,22 +105,25 @@ class imgRequestNotifyRunnable : public 
>     imgRequestNotifyRunnable(imgRequest* request, imgRequestProxy* requestproxy)
>       : mRequest(request)
>     {
>       mProxies.AppendElement(requestproxy);
>     }
> 
>     NS_IMETHOD Run()
>     {
>+      imgStatusTracker& statusTracker = mRequest->mImage ?
>+        mRequest->mImage->GetStatusTracker() : *mRequest->mStatusTracker;

Can't you just say mRequest->GetStatusTracker()?

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #16)
> >+  NS_ABORT_IF_FALSE(!mImage, "Setting null image");
> 
> I think this was supposed to be aImage here.

Indeed - thanks! (fixed in my patch queue)

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> This seems a little complex. How about:
> 
> if (mImage)
>   NS_ABORT_IF_FALSE(!mStatusTracker)
>   return mImage->GetStatusTracker()
> else
>   NS_ABORT_IF_FALSE(mStatusTracker)
>   NS_ABORT_IF_FALSE(!mImage)
>   return *mStatusTracker

Yup, that looks simpler. Fixed in patch queue. (Omitted the "!mImage" check, because it's already implied by the fact that we're in the |else| clause.)

> I think this'll fit on a single line, won't it?

Right - fixed.

> "use grab"

Fixed.

> Why not just GetStatusTracker().Notify()?

Fixed.

> Can't you just say mRequest->GetStatusTracker()?

Yes - fixed.

Thanks!

Comment 21

[Daniel Holbert [:dholbert]]

Landed:
part 1: http://hg.mozilla.org/mozilla-central/rev/0269abd2f98c
part 2: http://hg.mozilla.org/mozilla-central/rev/ba6bba1ff500
part 3: http://hg.mozilla.org/mozilla-central/rev/89e2f673c4cf