821023 - imgStatusTracker should not know about imgRequest

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

imgStatusTracker knows too much. Specifically, it knows which imgRequest it is associated with, and it grabs the current Image and the current imgStatusTracker from that imgRequest when it could manage that information itself. This makes it impossible to safely create images that wrap other images, which is needed for bug 790640.

The only risk associated with fixing this is that it might slightly change the notification behavior for multipart images. (In particular, it seems that async notifications for one part currently may be delivered by the imgStatusTracker associated with the next part if the switchover happens at the wrong time.) It doesn't seem like anything should be relying on this behavior, so this should be fine.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: imgStatusTracker should not know about imgRequest.

This patch removes all knowledge of imgRequest from imgStatusTracker.

Comment 2

[Seth Fowler [:seth] [:s2h]]

100% green try run for this: https://tbpl.mozilla.org/?tree=Try&rev=b58a1ded2d31

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 692120 [details] [diff] [review]
imgStatusTracker should not know about imgRequest.

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

I'm a *little* concerned that we're going from an ideologically pure world where imgStatusTrackers have exactly one owner to a loosy-goosy world where they're refcounted, but it's still probably the right direction. Nice work!

::: image/src/Image.h
@@ +152,5 @@
>  
>    uint64_t mInnerWindowId;
>  
>    // Member data shared by all implementations of this abstract class
> +  nsRefPtr<imgStatusTracker> mStatusTracker;

You need one more space before the "m" in this line.

::: image/src/RasterImage.cpp
@@ +351,5 @@
>                     imgIContainerDebug)
>  #endif
>  
>  //******************************************************************************
> +RasterImage::RasterImage(imgStatusTracker* aStatusTracker, nsIURI* aURI) :

Change the last argument here to

  nsIURI* aURI /* = nullptr */

for greater documentation.

::: image/src/VectorImage.cpp
@@ +168,5 @@
>  
>  //------------------------------------------------------------------------------
>  // Constructor / Destructor
>  
> +VectorImage::VectorImage(imgStatusTracker* aStatusTracker, nsIURI* aURI) :

Change the last argument here to
  nsIURI* aURI /* = nullptr */
for greater documentation.

::: image/src/imgRequestProxy.cpp
@@ +675,5 @@
> +void imgRequestProxy::OnStartDecode()
> +{
> +  // This notification is deliberately not propagated since there are no
> +  // listeners who care about it.
> + 

extra whitespace

@@ +681,5 @@
> +    /* In the case of streaming jpegs, it is possible to get multiple OnStartDecodes which
> +       indicates the beginning of a new decode.
> +       The cache entry's size therefore needs to be reset to 0 here.  If we do not do this,
> +       the code in imgStatusTrackerObserver::OnStopFrame will continue to increase the data size cumulatively.
> +    */

Can you make this a // C++ style comment?

@@ +750,5 @@
>      // Hold a ref to the listener while we call it, just in case.
>      nsCOMPtr<imgINotificationObserver> kungFuDeathGrip(mListener);
>      mListener->Notify(this, imgINotificationObserver::DISCARD, nullptr);
>    }
> + 

extra whitespace

::: image/src/imgRequestProxy.h
@@ +26,5 @@
> +{ /* 8bbe72b4-4991-4de4-a42a-4ab35f0efdbb */         \
> +     0x8bbe72b4,                                     \
> +     0x4991,                                         \
> +     0x4de4,                                         \
> +    {0xa4, 0x2a, 0x4a, 0xb3, 0x5f, 0x0e, 0xfd, 0xbb} \

You don't need to change CIDs when you're changing the interface; CIDs define *implementations*, not interface details.

Comment 4

[Seth Fowler [:seth] [:s2h]]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #3)

Thanks for the review, Joe!

> I'm a *little* concerned that we're going from an ideologically pure world
> where imgStatusTrackers have exactly one owner to a loosy-goosy world where
> they're refcounted, but it's still probably the right direction. Nice work!

Yeah, I would've avoided that if I could, but it didn't seem safe to let imgRequestNotifyRunnable hold a reference to the imgStatusTracker without making it refcounted.

> You don't need to change CIDs when you're changing the interface; CIDs
> define *implementations*, not interface details.

Cool, I wasn't sure so I did it just in case.

Will upload a new version with the requested fixes shortly.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: imgStatusTracker should not know about imgRequest.

Made changes requested in review. (No need for a new try run as everything is comment / whitespace / CID related.)

Comment 6

[Seth Fowler [:seth] [:s2h]]

Everything looks good. Requesting checkin.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38889f86a465

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Sorry, but something in bug 815471, bug 821023, bug 816374, or bug 816362 was causing reftest failures on all platforms and Android mochitest-8 failures. See the TBPL link below. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/47bd1f6fd8ed

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=22f0a7ee5348

Comment 9

[Seth Fowler [:seth] [:s2h]]

@Ryan, I think we've now confirmed that this is the patch causing the reftest failure. I'm investigating now.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: imgStatusTracker should not know about imgRequest.

Fixed a trivial bug (reversed boolean condition) that seems to have been responsible for the failures on try.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Looks 100% green on try now: https://tbpl.mozilla.org/?tree=Try&rev=d56196ae6ba8

Now that we have the fix in comment 10 I think this is ready to be checked in.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Crud, hold on, this needs a rebase. Let me do that and another try before checkin.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: imgStatusTracker should not know about imgRequest.

Rebased.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Try job for the rebased version here: https://tbpl.mozilla.org/?tree=Try&rev=7b69d44a3acd

Comment 15

[Seth Fowler [:seth] [:s2h]]

Try run looks good. Requesting checkin.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea9204be1bb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ea9204be1bb

Comment 18

[Justin Lebar (not reading bugmail)]

This patch removed the assignment to mURIString, but left the member and GetURIString(), both of which are always empty.  :(