| Summary: | <img crossorigin> should fail to load when CORS check fails | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | WebKit Review Bot <webkit.review.bot> | ||||||
| Component: | Page Loading | Assignee: | Adam Barth <abarth> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, bzbarsky, darin, japhet | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
WebKit Review Bot
2011-10-09 13:53:42 PDT
Created attachment 112119 [details]
work in progress
Created attachment 112130 [details]
Patch
Comment on attachment 112130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112130&action=review It seems a little strange to do the entire load and then report it as if it was a failure once the load is done. It seems the load should instead fail as soon as we get the HTTP header before reading any more data, loosely speaking. > Source/WebCore/loader/ImageLoader.cpp:243 > + if (m_element->fastHasAttribute(HTMLNames::crossoriginAttr) && !resource->passesAccessControlCheck(m_element->document()->securityOrigin())) { We normally do "using namespace HTMLNames" at the top of the file, and leave off the prefix. > It seems a little strange to do the entire load and then report it as if it was a failure once the load is done. It seems the load should instead fail as soon as we get the HTTP header before reading any more data, loosely speaking.
Yeah, we could do that. It's easier to do at this layer because it handles the cache-hit and cache-miss cases uniformly. I'd imagine the common case is for the check to pass though.
Comment on attachment 112130 [details] Patch Clearing flags on attachment: 112130 Committed r98215: <http://trac.webkit.org/changeset/98215> All reviewed patches have been landed. Closing bug. (In reply to comment #4) > > It seems a little strange to do the entire load and then report it as if it was a failure once the load is done. It seems the load should instead fail as soon as we get the HTTP header before reading any more data, loosely speaking. > > Yeah, we could do that. It's easier to do at this layer because it handles the cache-hit and cache-miss cases uniformly. I'd imagine the common case is for the check to pass though. The CachedResource class could be changed call a function that says “we have the header but not the data” for both the cache-miss case and the cache-hit case. For the cache-hit case it would be called right before the call to notifyFinished. What I like about this is not just the performance but the clarity that processing of the header goes in the header function and processing of the complete data and header goes in the data function and processing of partial data goes in the partial data function. Obviously notifyFinished could use a better name too. Yeah, that approach is consistent with CachedResource's design of synchronously generating the various callbacks during a cache hit in addClient. I'll keep this issue in mind when looking at Nate's patch to make XMLHttpRequest over to the CachedResourceLoader pipeline. XMLHttpRequest needs a full-featured CORS implementation, so it should be easier for ImageLoader to do this work once that's done. Adam, should I expect the fix for this bug to show up in this Chrome version: Google Chrome 17.0.942.0 (Official Build 110446) dev OS Mac OS X WebKit 535.8 (@100508) ? As far as I can tell that Chrome version is loading the various images linked from the list on the left of http://maps.google.it/maps/ms?msid=211095647232644570760.00047466b28e95f25449b&msa=0 that use @crossorigin but don't send CORS headers.... Thanks Boris. I'll investigate. |