Summary: | Update ImageDecoder::create to match sniffing spec | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||||||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, dglazkov, eric, gustavo, pascal.massimino, pkasting, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Other | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Attachments: |
|
Description
Adam Barth
2010-10-14 13:22:32 PDT
Created attachment 70769 [details]
Patch
Attachment 70769 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4436039 Comment on attachment 70769 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=70769&action=review Is walking a vector char-by-char really better than memcmp()? LGTM otherwise > WebCore/platform/image-decoders/ImageDecoder.cpp:58 > + for (unsigned i = 0; i < pattern.length(); ++i) { This is going to have trouble with the WEBP signature since that has a hole in the middle. Do you have a plan for how you'll extend it? > WebCore/platform/image-decoders/ImageDecoder.cpp:73 > + DEFINE_STATIC_LOCAL(Vector<char>, GIFSignature1, ("GIF87a", 6)); Nit: I think it'd be a little clearer to move these to the top of the function and define maxSignatureLength next to them. > WebCore/platform/image-decoders/ImageDecoder.cpp:79 > + DEFINE_STATIC_LOCAL(Vector<char>, ICOSignature2, ("\x00\x00\x02\x00", 4)); Nit: I suggest "CURSignature" instead for clarity. > WebCore/platform/image-decoders/ImageDecoder.cpp:81 > + if (matcheSignature(GIFSignature1, contents) || matcheSignature(GIFSignature2, contents)) Won't compile: matcheSignature -> matchesSignature > WebCore/platform/image-decoders/ImageDecoder.cpp:93 > // ICOs always begin with a 2-byte 0 followed by a 2-byte 1. Nit: These comments aren't really needed. Is this observable from JS? could we create some bogus binary files which match these signatures and find out how the engine is sniffing them? (In reply to comment #4) > Is this observable from JS? Not as far as I know. There's unlikely to even be theoretical impact given that things like the GIF signatures were already being checked in GIFImageDecoder.cpp. Attachment 70769 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4466044 > Is walking a vector char-by-char really better than memcmp()? It's more obviously memory safe. > > WebCore/platform/image-decoders/ImageDecoder.cpp:58 > > + for (unsigned i = 0; i < pattern.length(); ++i) { > > This is going to have trouble with the WEBP signature since that has a hole in the middle. Do you have a plan for how you'll extend it? I actually wrote the patch originally with a mask parameter too, but then I removed it because it's not needed yet. > > WebCore/platform/image-decoders/ImageDecoder.cpp:73 > > + DEFINE_STATIC_LOCAL(Vector<char>, GIFSignature1, ("GIF87a", 6)); > > Nit: I think it'd be a little clearer to move these to the top of the function and define maxSignatureLength next to them. Ok. > > WebCore/platform/image-decoders/ImageDecoder.cpp:79 > > + DEFINE_STATIC_LOCAL(Vector<char>, ICOSignature2, ("\x00\x00\x02\x00", 4)); > > Nit: I suggest "CURSignature" instead for clarity. Ah, good idea. > > WebCore/platform/image-decoders/ImageDecoder.cpp:81 > > + if (matcheSignature(GIFSignature1, contents) || matcheSignature(GIFSignature2, contents)) > > Won't compile: matcheSignature -> matchesSignature I guess this file isn't used in the Mac build. > > WebCore/platform/image-decoders/ImageDecoder.cpp:93 > > // ICOs always begin with a 2-byte 0 followed by a 2-byte 1. > > Nit: These comments aren't really needed. Yeah, especially once I rename the signatures as you suggest. Created attachment 70779 [details]
Patch
Attachment 70779 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4467019 Attachment 70779 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4419034 Comment on attachment 70779 [details]
Patch
I'll make it compile before landing.
I looked into creating a test for this. It turns out that Drag and drop knows the extension of the image. So if we dragged the image off screen we could get its extension. JavaScript however has no way to get the extension (or mimetype) of a displayed image as far as I can tell. Created attachment 70799 [details]
the test case I was playing around with (for posterity)
Comment on attachment 70779 [details]
Patch
I really wish we had better testing for our image decoders.
Created attachment 72767 [details]
merge to TOT
Created attachment 72784 [details]
merge to TOT
(In reply to comment #16) > Created an attachment (id=72784) [details] > merge to TOT shouldn't the WEBSignature be: "RIFF????WEBPVP" (14 symbols, with four '?'s) ? > shouldn't the WEBSignature be: "RIFF????WEBPVP" (14 symbols, with four '?'s) ?
That probably explains why the WebP test is failing. :)
Created attachment 94439 [details]
Patch
Comment on attachment 94439 [details]
Patch
OK. Seems we could test this by having a green square PNG and modifying its signature, or similarly a GIF. And make sure that it loads or doesn't load.
Possibly, unless something else in the network stack obscures that difference. I can try writing such a test. Created attachment 94443 [details]
Patch
Comment on attachment 94443 [details]
Patch
OK.
Comment on attachment 94443 [details] Patch Clearing flags on attachment: 94443 Committed r87090: <http://trac.webkit.org/changeset/87090> All reviewed patches have been landed. Closing bug. |