Bug 163242 – ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString()

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

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()

[Ryan Haddad]

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

[Ryan Haddad]

Looks like this assert was added with https://trac.webkit.org/changeset/206995

[youenn fablet]

Thanks for filing this bug.

I cannot reproduce it on my local machine though :(
I will continue monitor these two tests.

[youenn fablet]

Also crashing in http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html
Will further study.

[youenn fablet]

Created attachment 291939 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

(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).

[Darin Adler]

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.

[youenn fablet]

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

[youenn fablet]

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.

[youenn fablet]

Created attachment 292592 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292592 [details]
Patch for landing

Clearing flags on attachment: 292592

Committed r207754: <http://trac.webkit.org/changeset/207754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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.

[Darin Adler]

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.

[youenn fablet]

Filed bug 163938 to improve the efficiency of originsMatch