imgIDecoder shouldn't use an nsIInputStream for input

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

Trunk
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

10 years ago
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.

Updated

10 years ago
Summary: imgIDecoder shouldn't use an nsIInputString for input → imgIDecoder shouldn't use an nsIInputStream for input
Assignee

Comment 1

10 years ago
Posted patch patch v1 (obsolete) — Splinter Review
added a patch for this. I'll flag for review once things settle down with bug 435296.
Assignee: nobody → bobbyholley
Status: NEW → ASSIGNED
Assignee

Updated

10 years ago
Blocks: 513681
Assignee

Comment 2

10 years ago
Posted patch patch v2 (obsolete) — Splinter Review
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.
Attachment #396829 - Attachment is obsolete: true
Assignee

Comment 3

10 years ago
Comment on attachment 406540 [details] [diff] [review]
patch v2

try looks good. flagging joe for review.
Attachment #406540 - Flags: review?(joe)
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.
Attachment #406540 - Flags: review?(joe) → review+
Assignee

Comment 5

10 years ago
Posted patch patch v3Splinter Review
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.
Attachment #406540 - Attachment is obsolete: true
Assignee

Comment 6

10 years ago
pushed to mozilla-central as 1d5e6291b258.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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...
Crap, yes. Bobby: can you back this out and flag vlad for sr?
Assignee

Comment 9

10 years ago
(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.
Assignee

Comment 10

10 years ago
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 on attachment 406618 [details] [diff] [review]
patch v3

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

Comment 12

10 years ago
pushed the uuid rev to mc in 431c83030afe.
You need to log in before you can comment on or make changes to this bug.