| Summary: | ASSERTION FAILED: canvas()->securityOrigin()->toString() == cachedImage.origin()->toString() | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||
| Component: | New Bugs | Assignee: | youenn fablet <youennf> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, commit-queue, darin, dbates, japhet, youennf | ||||||||
| Priority: | P2 | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Ryan Haddad
2016-10-10 14:38:38 PDT
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 |