656351 - Refactor mDecoder null-checks in image/RasterImage.cpp

Status: RESOLVED INACTIVE

(Core :: Graphics: ImageLib, enhancement)

Description

[Mats Palmgren (inactive)]

Followup from bug 639303 comment 36:

Joe Drew (:JOEDREW!) 2011-04-21 12:17:19 PDT:

>@@ -2406,28 +2418,29 @@ RasterImage::SyncDecode()

>-  if (IsDecodeFinished()) {
>+  if (mDecoder && IsDecodeFinished()) {
>     rv = ShutdownDecoder(eShutdownIntent_Done);
>     CONTAINER_ENSURE_SUCCESS(rv);
>   }

>@@ -2688,17 +2703,17 @@ imgDecodeWorker::Run()

>   // If the decode finished, shutdown the decoder
>-  if (image->IsDecodeFinished()) {
>+  if (image->mDecoder && image->IsDecodeFinished()) {

These two null checks should be moved inside IsDecodeFinished or ShutdownDecoder, or wherever they're necessary. I'm OK with it if you do that as a followup bug.

Comment 1

[Joe Drew (not getting mail)]

These checks are located here: http://hg.mozilla.org/mozilla-central/file/49936b49aff3/image/src/RasterImage.cpp#l2539
and here: http://hg.mozilla.org/mozilla-central/file/49936b49aff3/image/src/RasterImage.cpp#l2855

At minimum, this bug needs us to move the mDecoder null check into RasterImage::IsDecodeFinished and/or ShutdownDecoder, so we don't use it when it's NULL.

Comment 2

[Nguyen Ngoc Trung]

Attachment: Propose patch

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 606955 [details] [diff] [review]
Propose patch

Thanks for the patch, Nguyen! You should ask for a review of future patches by visiting the Details link on the attachment, setting the review field to ? and adding an email address of name of a reviewer.

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 606955 [details] [diff] [review]
Propose patch

>diff -r e94edfdb1f5b image/src/RasterImage.cpp
> bool
> RasterImage::IsDecodeFinished()
> {
>   // Precondition
>   NS_ABORT_IF_FALSE(mDecoder, "Can't call IsDecodeFinished() without decoder!");
>+	
>+  // If decoder is null 
>+  if (!mDecoder) 
>+    return false;

I'm not joedrew (and not a real reviewer here) but NS_ABORT_IF_FALSE will ensure you will never reach your new condition. My (personal) recommendation is to delete that line and its comment because we're now caring about it inside this function rather than outside.

Also comment 0 in this bug suggests there is/may be another |mDecoder| check elsewhere to drop.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 606955 [details] [diff] [review]
Propose patch

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

Nguyen Nogc Trung, you'll need to remove the NS_ABORT_IF_FALSE above the if() you added; as it is, we have two logically inconsistent blocks.

If you can upload an patch with that changed, I'll be very glad to get it in to Mozilla!

Comment 6

[Nguyen Ngoc Trung]

Attachment: Updated patch with new changes.

I have gotten rid of the precondition as you said.

Comment 7

[Justin Lebar (not reading bugmail)]

Be sure to mark the reviewer on patches you'd like reviewed, otherwise we're likely to miss them (as we apparently did this time).

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 612111 [details] [diff] [review]
Updated patch with new changes.

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

::: image/src/RasterImage.cpp
@@ +2734,5 @@
>  // This method may not be called if there is no decoder.
>  bool
>  RasterImage::IsDecodeFinished()
> +{	
> +  // If decoder is null 

unnecessary comment :)

Comment 9

[Matt Brubeck (:mbrubeck)]

It looks like this patch was reviewed but never checked in.  At last part of it still applies; can it be updated and checked in?

Comment 10

[Joe Drew (not getting mail)]

Trung, can you update the patch to remove that comment and re-upload it? Then we can check it in!

Comment 11

[Joe Drew (not getting mail)]

Whoops, not sure what happened there.

Comment 12

[Nick Lebedev [:nl]]

Hi,

I would like to complete this bug. Is the work we would like to complete in this bug still in force? I saw that code touched by the patch slightly changed. It mostly relates to the ending of the method RasterImage::SyncDecode().

Comment 13

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

So I'm actually in the process of refactoring related stuff right now. This seems like a good followup for bug 1079653, which I hope to land today.

Also, not sure why this got marked ASSIGNED with an empty "Assigned To" field. Nick, do you want this to be assigned to you? If so, please set the field appropriately. If not, I'll be happy to take this over.

Comment 14

[Nick Lebedev [:nl]]

Yes, i would like to take the bug. I just forget to put me into 'assign to' field :)

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.