512269 - imgIDecoder shouldn't use an nsIInputStream for input

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

After bug 435296 lands, there's no reason for imgIDecoder to accept an nsIInputStream, and continuing to do so just means a lot of boilerplate code (duplicated through all the decoders). We should make writing to decoders [noscript], and have them accept a buffer and a length.

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch v1

added a patch for this. I'll flag for review once things settle down with bug 435296.

Comment 2

[Bobby Holley (:bholley)]

Attachment: patch v2

Updated the patch to fix a bunch of bitrot. I decided that to make better progress on bug 516334 it would be nice if things were centralized with bug 513681, and this bug blocks that one.

pushed to try as daf74129697f.

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 406540 [details] [diff] [review]
patch v2

try looks good. flagging joe for review.

Comment 4

[Joe Drew (not getting mail)]

Comment on attachment 406540 [details] [diff] [review]
patch v2

Do we have tests for trying to decode invalid images? I don't want another example like bug 519589 to show up, and this patch causes the decoders to have a lot of early-exit cases now. 

>+    // Determine if we want to salvage the situation
>+    PRUint32 numFrames = 0;
>+    if (mImageContainer)
>+      mImageContainer->GetNumFrames(&numFrames);
>+
>+    // If we're salvaging, send off notifications
>+    if (numFrames > 1) { // XXXbholley - this is from the old code, but why not > 0?
>+      EndGIF(/* aSuccess = */ PR_TRUE);
>+    }

imgContainer's numFrames will be 1 as soon as we try to decode the first frame, since AppendFrame immediately creates and adds an imgFrame to its array. This doesn't mean that the frame was decoded properly, though. We only know that we decoded the first frame properly because we've reached the second frame.

Also, if one doesn't already exist, can you either add a test or file a bug for a test that ensures this behaviour?

>   case JPEG_ERROR:
>+    return NS_ERROR_FAILURE;
>     PR_LOG(gJPEGlog, PR_LOG_DEBUG,
>            ("[this=%p] nsJPEGDecoder::ProcessData -- entering JPEG_ERROR case\n", this));
> 
>     break;
>   }

We should either have a return at the end of the case, and no break, or no return. (As is, we also bypass the PR_LOG.) Also, is it safe to have this unconditional return? I suppose we're just refusing to process the rest of the data, so it's probably OK.

Comment 5

[Bobby Holley (:bholley)]

Attachment: patch v3

Updated patch.

(In reply to comment #4)

> Also, if one doesn't already exist, can you either add a test or file a bug for
> a test that ensures this behaviour?

Filed bug 522629.

Comment 6

[Bobby Holley (:bholley)]

pushed to mozilla-central as 1d5e6291b258.

Comment 7

[Shawn Wilsher :sdwilsh]

Shouldn't this have gotten sr before it landed per the new sr policy?  I see you changed an idl file and therefore changed a public interface...

Comment 8

[Joe Drew (not getting mail)]

Crap, yes. Bobby: can you back this out and flag vlad for sr?

Comment 9

[Bobby Holley (:bholley)]

(In reply to comment #7)
> Shouldn't this have gotten sr before it landed per the new sr policy?  I see
> you changed an idl file and therefore changed a public interface...

Well, it's a gray area. imgIDecoder is technically a public interface, but it's actually 100% internal to imagelib. In theory there could be external decoders, but there aren't any. Let me find someone of authority to ask.

Comment 10

[Bobby Holley (:bholley)]

roc just said an after-the-fact sr from him is ok here and gave a verbal sr.

I do however need to rev the UUID. patch coming up.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 406618 [details] [diff] [review]
patch v3

post-facto sr+, but you need to rev the IID on imgIDecoder

Comment 12

[Bobby Holley (:bholley)]

pushed the uuid rev to mc in 431c83030afe.