Encountered on El Capitan Debug WK2 with LayoutTest http/tests/security/canvas-remote-read-data-url-image.html https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r206999%20(8941)/results.html ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString() /Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/html/canvas/CanvasRenderingContext.cpp(72) : bool WebCore::CanvasRenderingContext::wouldTaintOrigin(const WebCore::HTMLImageElement *) 1 0x101db6790 WTFCrash 2 0x10720c6c4 WebCore::CanvasRenderingContext::wouldTaintOrigin(WebCore::HTMLImageElement const*) 3 0x10721cd8f void WebCore::CanvasRenderingContext::checkOrigin<WebCore::HTMLImageElement>(WebCore::HTMLImageElement const*) 4 0x1072140b8 WebCore::CanvasRenderingContext2D::drawImage(WebCore::HTMLImageElement&, WebCore::FloatRect const&, WebCore::FloatRect const&, WebCore::CompositeOperator const&, WebCore::BlendMode const&, int&) 5 0x1072134fc WebCore::CanvasRenderingContext2D::drawImage(WebCore::HTMLImageElement&, WebCore::FloatRect const&, WebCore::FloatRect const&, int&) 6 0x107213465 WebCore::CanvasRenderingContext2D::drawImage(WebCore::HTMLImageElement&, float, float, float, float, int&) 7 0x10804ea69 WebCore::jsCanvasRenderingContext2DPrototypeFunctionDrawImage2(JSC::ExecState*) 8 0x10803ceec WebCore::jsCanvasRenderingContext2DPrototypeFunctionDrawImage(JSC::ExecState*) 9 0x2b3f63a01028 10 0x10198d5a4 llint_entry 11 0x1019860be vmEntryToJavaScript 12 0x1017677dc JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) 13 0x1016e503f JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 14 0x100f32b9e JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 15 0x100f32c79 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 16 0x100f32ebb JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 17 0x10802e5fb WebCore::JSMainThreadExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) 18 0x1082bdac4 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) 19 0x10786ca83 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener>, 1ul, WTF::CrashOnOverflow, 16ul>) 20 0x10786c648 WebCore::EventTarget::fireEventListeners(WebCore::Event&) 21 0x108c45984 WebCore::Node::handleLocalEvents(WebCore::Event&) 22 0x10783b991 WebCore::EventContext::handleLocalEvents(WebCore::Event&) const 23 0x10783c7bc WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&, WebCore::WindowEventContext&) 24 0x10783c4c8 WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WebCore::Event&) 25 0x108c459dd WebCore::Node::dispatchEvent(WebCore::Event&) 26 0x107c18921 WebCore::HTMLImageLoader::dispatchLoadEvent() 27 0x107e27835 WebCore::ImageLoader::dispatchPendingLoadEvent() 28 0x107e27756 WebCore::ImageLoader::dispatchPendingEvent(WebCore::EventSender<WebCore::ImageLoader>*) 29 0x107e27eea WebCore::EventSender<WebCore::ImageLoader>::dispatchPendingEvents() 30 0x107e27951 WebCore::ImageLoader::dispatchPendingLoadEvents() 31 0x10768dc77 WebCore::Document::implicitClose()
Also seen here with imported/w3c/web-platform-tests/html/semantics/embedded-content/the-canvas-element/security.dataURI.html https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK1%20(Tests)/r207013%20(16666)/results.html
Looks like this assert was added with https://trac.webkit.org/changeset/206995
Thanks for filing this bug. I cannot reproduce it on my local machine though :( I will continue monitor these two tests.
Also crashing in http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html Will further study.
Created attachment 291939 [details] Patch
Comment on attachment 291939 [details] Patch Attachment 291939 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2313105 New failing tests: svg/as-image/svg-image-with-data-uri-use-data-uri.svg
Created attachment 291945 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #6) > Comment on attachment 291939 [details] > Patch > > Attachment 291939 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/2313105 > > New failing tests: > svg/as-image/svg-image-with-data-uri-use-data-uri.svg The failure does not seem related to the patch. This test is flaky on my machine with and without the patch. On the debug bot, it is crashing in Source/WebCore/svg/graphics/SVGImage.cpp on ASSERT(observer).
Comment on attachment 291939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291939&action=review The crash in svg/as-image/svg-image-with-data-uri-use-data-uri.svg seems like it might be caused by this patch. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:543 > +static inline bool isRequestMatchingResourceOrigin(const CachedResourceRequest& request, const CachedResource& resource) I would call this function "originsMatch", not "isRequestMatchingResourceOrigin". The algorithm it implements is symmetric so there is no need to name it the way we did. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:549 > + return request.origin()->toString() == resource.origin()->toString(); Is comparing the strings really the correct and efficient way to correctly compare origins? There must be a more efficient way to implement the same operation with help from the origin class itself.
Thanks for the review. (In reply to comment #9) > Comment on attachment 291939 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291939&action=review > > The crash in svg/as-image/svg-image-with-data-uri-use-data-uri.svg seems > like it might be caused by this patch. > > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:543 > > +static inline bool isRequestMatchingResourceOrigin(const CachedResourceRequest& request, const CachedResource& resource) > > I would call this function "originsMatch", not > "isRequestMatchingResourceOrigin". The algorithm it implements is symmetric > so there is no need to name it the way we did. OK > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:549 > > + return request.origin()->toString() == resource.origin()->toString(); > > Is comparing the strings really the correct and efficient way to correctly > compare origins? There must be a more efficient way to implement the same > operation with help from the origin class itself. I use string comparison here as this is is how gets serialized as a HTTP header. This is in particular important for
Thanks for the review. (In reply to comment #9) > Comment on attachment 291939 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=291939&action=review > > The crash in svg/as-image/svg-image-with-data-uri-use-data-uri.svg seems > like it might be caused by this patch. I will keep looking at this test. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:543 > > +static inline bool isRequestMatchingResourceOrigin(const CachedResourceRequest& request, const CachedResource& resource) > > I would call this function "originsMatch", not > "isRequestMatchingResourceOrigin". The algorithm it implements is symmetric > so there is no need to name it the way we did. OK > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:549 > > + return request.origin()->toString() == resource.origin()->toString(); > > Is comparing the strings really the correct and efficient way to correctly > compare origins? There must be a more efficient way to implement the same > operation with help from the origin class itself. string comparison is used here as toString() is how gets serialized as a HTTP header. This is in particular important when the origin gets serialised as "null". If both origins are serialised as null, we want to reuse the resource (marked as cross origin in both cases) I will add a comment about it in the code.
Created attachment 292592 [details] Patch for landing
Comment on attachment 292592 [details] Patch for landing Clearing flags on attachment: 292592 Committed r207754: <http://trac.webkit.org/changeset/207754>
All reviewed patches have been landed. Closing bug.
I filed bug 163887 to keep track of svg/as-image/svg-image-with-data-uri-use-data-uri.svg crashes which are confirmed in debug bots.
Comment on attachment 291939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291939&action=review >>>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:549 >>>> + return request.origin()->toString() == resource.origin()->toString(); >>> >>> Is comparing the strings really the correct and efficient way to correctly compare origins? There must be a more efficient way to implement the same operation with help from the origin class itself. >> >> I use string comparison here as this is is how gets serialized as a HTTP header. >> This is in particular important for > > string comparison is used here as toString() is how gets serialized as a HTTP header. > This is in particular important when the origin gets serialised as "null". > If both origins are serialised as null, we want to reuse the resource (marked as cross origin in both cases) > > I will add a comment about it in the code. That’s a fine answer about correctness but it does not answer my question about efficiency. I understand that this must return the correct result as if we converted to a string.
Filed bug 163938 to improve the efficiency of originsMatch