Async loading of image resources
Created attachment 234157 [details] Patch
Comment on attachment 234157 [details] Patch This patch is far from being ready for review. I've uploaded it in order to be able to discuss its technical approach
Attachment 234157 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 234157 [details] Patch Attachment 234157 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6295351275290624 New failing tests: canvas/philip/tests/2d.composite.canvas.destination-in.html canvas/philip/tests/2d.composite.canvas.source-out.html canvas/philip/tests/2d.composite.clip.copy.html canvas/philip/tests/2d.composite.canvas.copy.html accessibility/alt-tag-on-image-with-nonimage-role.html canvas/philip/tests/2d.clearRect.zero.html canvas/philip/tests/2d.clearRect.globalalpha.html canvas/philip/tests/2d.composite.canvas.source-in.html canvas/philip/tests/2d.composite.canvas.destination-out.html canvas/philip/tests/2d.clearRect.globalcomposite.html canvas/philip/tests/2d.clearRect.clip.html canvas/philip/tests/2d.composite.canvas.source-atop.html canvas/philip/tests/2d.composite.clip.destination-in.html canvas/philip/tests/2d.clearRect.shadow.html canvas/philip/tests/2d.composite.clip.destination-over.html canvas/philip/tests/2d.clearRect.transform.html canvas/philip/tests/2d.clearRect.negative.html canvas/philip/tests/2d.clearRect.path.html canvas/philip/tests/2d.composite.clip.destination-atop.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html canvas/philip/tests/2d.clearRect.basic.html canvas/philip/tests/2d.composite.canvas.destination-over.html canvas/philip/tests/2d.composite.canvas.destination-atop.html canvas/philip/tests/2d.composite.clip.destination-out.html canvas/philip/tests/2d.composite.canvas.lighter.html canvas/philip/tests/2d.composite.canvas.xor.html canvas/philip/tests/2d.composite.canvas.source-over.html canvas/philip/tests/2d.clearRect+fillRect.basic.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html canvas/philip/tests/2d.clearRect.nonfinite.html
Created attachment 234160 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 234157 [details] Patch Attachment 234157 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5532361072574464 New failing tests: canvas/philip/tests/2d.composite.canvas.destination-in.html canvas/philip/tests/2d.composite.canvas.source-out.html canvas/philip/tests/2d.composite.clip.copy.html canvas/philip/tests/2d.composite.canvas.copy.html accessibility/alt-tag-on-image-with-nonimage-role.html canvas/philip/tests/2d.clearRect.zero.html canvas/philip/tests/2d.clearRect.globalalpha.html canvas/philip/tests/2d.composite.canvas.source-in.html canvas/philip/tests/2d.composite.canvas.destination-out.html canvas/philip/tests/2d.clearRect.globalcomposite.html canvas/philip/tests/2d.clearRect.clip.html canvas/philip/tests/2d.composite.canvas.source-atop.html canvas/philip/tests/2d.composite.clip.destination-in.html canvas/philip/tests/2d.clearRect.shadow.html canvas/philip/tests/2d.composite.clip.destination-over.html canvas/philip/tests/2d.clearRect.transform.html canvas/philip/tests/2d.clearRect.negative.html canvas/philip/tests/2d.clearRect.path.html canvas/philip/tests/2d.composite.clip.destination-atop.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html canvas/philip/tests/2d.clearRect.basic.html canvas/philip/tests/2d.composite.canvas.destination-over.html canvas/philip/tests/2d.composite.canvas.destination-atop.html canvas/philip/tests/2d.composite.clip.destination-out.html canvas/philip/tests/2d.composite.canvas.lighter.html canvas/philip/tests/2d.composite.canvas.xor.html canvas/philip/tests/2d.composite.canvas.source-over.html canvas/philip/tests/2d.clearRect+fillRect.basic.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html canvas/philip/tests/2d.clearRect.nonfinite.html
Created attachment 234163 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 234157 [details] Patch Attachment 234157 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4580072602206208 New failing tests: canvas/philip/tests/2d.composite.canvas.destination-in.html canvas/philip/tests/2d.composite.canvas.source-out.html canvas/philip/tests/2d.composite.clip.copy.html canvas/philip/tests/2d.composite.canvas.copy.html accessibility/alt-tag-on-image-with-nonimage-role.html canvas/philip/tests/2d.clearRect.zero.html canvas/philip/tests/2d.clearRect.globalalpha.html canvas/philip/tests/2d.composite.canvas.source-in.html canvas/philip/tests/2d.composite.canvas.destination-out.html canvas/philip/tests/2d.clearRect.globalcomposite.html canvas/philip/tests/2d.clearRect.clip.html canvas/philip/tests/2d.composite.canvas.source-atop.html canvas/philip/tests/2d.composite.clip.destination-in.html canvas/philip/tests/2d.clearRect.shadow.html canvas/philip/tests/2d.composite.clip.destination-over.html canvas/philip/tests/2d.clearRect.transform.html canvas/philip/tests/2d.clearRect.negative.html canvas/philip/tests/2d.clearRect.path.html canvas/philip/tests/2d.composite.clip.destination-atop.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html canvas/philip/tests/2d.clearRect.basic.html canvas/philip/tests/2d.composite.canvas.destination-over.html canvas/philip/tests/2d.composite.canvas.destination-atop.html canvas/philip/tests/2d.composite.clip.destination-out.html canvas/philip/tests/2d.composite.canvas.lighter.html canvas/philip/tests/2d.composite.canvas.xor.html canvas/philip/tests/2d.composite.canvas.source-over.html canvas/philip/tests/2d.clearRect+fillRect.basic.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html canvas/philip/tests/2d.clearRect.nonfinite.html
Created attachment 234164 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 239023 [details] Patch
Added a microtask abstraction, and register the image loading off of that. This is not yet destined for review. Uploading just to test it on the bots.
Created attachment 239044 [details] Patch
Comment on attachment 239044 [details] Patch Attachment 239044 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4828100001005568 New failing tests: dom/xhtml/level2/html/HTMLDocument03.xhtml dom/xhtml/level2/html/HTMLDocument16.xhtml dom/xhtml/level2/html/HTMLDocument07.xhtml dom/xhtml/level2/html/HTMLDocument13.xhtml http/tests/history/popstate-fires-with-pending-requests.html css3/flexbox/relayout-image-load.html canvas/philip/tests/2d.pattern.image.incomplete.empty.html canvas/philip/tests/2d.drawImage.incomplete.html dom/xhtml/level2/html/HTMLDocument05.xhtml dom/xhtml/level2/html/HTMLDocument01.xhtml dom/xhtml/level2/html/HTMLDocument08.xhtml dom/xhtml/level2/html/HTMLDocument11.xhtml dom/xhtml/level2/html/HTMLDocument10.xhtml dom/xhtml/level2/html/HTMLDocument15.xhtml dom/xhtml/level2/html/HTMLBodyElement09.xhtml dom/xhtml/level2/html/HTMLDocument14.xhtml dom/xhtml/level2/html/HTMLDocument04.xhtml dom/xhtml/level2/html/HTMLDocument09.xhtml dom/xhtml/level2/html/HTMLBodyElement10.xhtml dom/xhtml/level2/html/HTMLDocument02.xhtml dom/xhtml/level2/html/HTMLBodyElement12.xhtml dom/xhtml/level2/html/HTMLBodyElement07.xhtml canvas/philip/tests/2d.pattern.image.incomplete.omitted.html compositing/video/video-poster.html dom/xhtml/level2/html/HTMLBodyElement11.xhtml css2.1/t0801-c412-hz-box-00-b-a.html dom/xhtml/level2/html/HTMLDocument12.xhtml canvas/philip/tests/2d.pattern.image.incomplete.html canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html dom/xhtml/level2/html/HTMLBodyElement08.xhtml
Created attachment 239058 [details] Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 239044 [details] Patch Attachment 239044 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4579083971198976 New failing tests: dom/xhtml/level2/html/HTMLDocument20.xhtml dom/xhtml/level2/html/HTMLDocument03.xhtml dom/xhtml/level2/html/HTMLDocument16.xhtml dom/xhtml/level2/html/HTMLDocument07.xhtml dom/xhtml/level2/html/HTMLDocument13.xhtml http/tests/history/popstate-fires-with-pending-requests.html dom/xhtml/level2/html/HTMLBodyElement09.xhtml dom/xhtml/level2/html/HTMLDocument05.xhtml dom/xhtml/level2/html/HTMLDocument01.xhtml dom/xhtml/level2/html/HTMLDocument08.xhtml dom/xhtml/level2/html/HTMLDocument11.xhtml dom/xhtml/level2/html/HTMLDocument10.xhtml dom/xhtml/level2/html/HTMLDocument15.xhtml dom/xhtml/level2/html/HTMLDocument19.xhtml canvas/philip/tests/2d.drawImage.incomplete.html dom/xhtml/level2/html/HTMLDocument14.xhtml dom/xhtml/level2/html/HTMLDocument04.xhtml dom/xhtml/level2/html/HTMLDocument09.xhtml dom/xhtml/level2/html/HTMLDocument18.xhtml dom/xhtml/level2/html/HTMLBodyElement10.xhtml dom/xhtml/level2/html/HTMLDocument02.xhtml dom/xhtml/level2/html/HTMLBodyElement12.xhtml dom/xhtml/level2/html/HTMLBodyElement07.xhtml dom/xhtml/level2/html/HTMLBodyElement11.xhtml dom/xhtml/level2/html/HTMLDocument17.xhtml dom/xhtml/level2/html/HTMLDocument12.xhtml dom/xhtml/level2/html/HTMLDocument21.xhtml css2.1/t0801-c412-hz-box-00-b-a.html canvas/philip/tests/2d.drawImage.image.incomplete.omitted.html dom/xhtml/level2/html/HTMLBodyElement08.xhtml
Created attachment 239061 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 239322 [details] Patch
Created attachment 239325 [details] Patch
Comment on attachment 239325 [details] Patch This version of the patch seems to be working, but I still need to add tests before I can submit it for official review.
Created attachment 239327 [details] Patch
Comment on attachment 239327 [details] Patch Attachment 239327 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6256222529388544 Number of test failures exceeded the failure limit.
Created attachment 239329 [details] Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 239327 [details] Patch Attachment 239327 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6636140304007168 Number of test failures exceeded the failure limit.
Created attachment 239332 [details] Archive of layout-test-results from webkit-ews-03 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-03 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 248223 [details] Patch
Comment on attachment 248223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248223&action=review > Source/WebCore/loader/ImageLoader.h:131 > + class ImageLoaderTask : public MicroTask { > + public: > + static std::unique_ptr<ImageLoaderTask> create(WeakPtr<ImageLoader> loader) > + { > + return std::unique_ptr<ImageLoaderTask>(new ImageLoaderTask(loader)); > + } > + > + virtual void run() > + { > + if (m_loader && !m_loader->cancelledTask()) > + m_loader->doUpdateFromElement(); > + } > + > + private: > + ImageLoaderTask(WeakPtr<ImageLoader> loader) > + : m_loader(loader) > + { > + } > + > + WeakPtr<ImageLoader> m_loader; > + }; There’s no need to have this whole class in the header.
Thanks! I'll pull it out of there. This patch is not yet in good shape for review. I mainly uploaded it to see if the tests are passing on the bots.
Comment on attachment 248223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248223&action=review > Source/WebCore/loader/ImageLoader.h:49 > + WTF_MAKE_NONCOPYABLE(IncrementLoadEventDelayCount); I’m not sure this needs to be here. > Source/WebCore/loader/ImageLoader.h:57 > + IncrementLoadEventDelayCount() { }; > + IncrementLoadEventDelayCount(IncrementLoadEventDelayCount&& other) > + : m_document(WTF::move(other.m_document)) > + { > + other.m_document.clear(); > + } These function bodies don’t need to be in the header. Also, there’s no need to declare the empty constructor at all. The code here to move a WeakPtr should be put into the WeakPtr template. If that was done correctly, then we would not need to explicitly define a move constructor at all. > Source/WebCore/loader/ImageLoader.h:67 > + WeakPtr<Document> m_document; I am not sure we need a WeakPtr here. I believe that callers already handle the lifetime for the document properly and destroy the image loader before the document is destroyed; this WeakPtr would add unnecessary overhead.
Comment on attachment 248223 [details] Patch Attachment 248223 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5543544144003072 Number of test failures exceeded the failure limit.
Created attachment 248225 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248223 [details] Patch Attachment 248223 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6608190301011968 Number of test failures exceeded the failure limit.
Created attachment 248226 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249397 [details] Patch
I've uploaded a new patch that works (and passes tests), at least locally. It includes a few additional `runMicroTasks()` calls after events and scheduled JS actions. I'm not sure these are required by spec, but without them, some images fail to load in certain tested scenarios. I'm also not sure if and how they should be protected against nested call scenarios. Advice would be appreciated.
Created attachment 249398 [details] Patch
Comment on attachment 249398 [details] Patch Attachment 249398 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5233336238735360 Number of test failures exceeded the failure limit.
Created attachment 249399 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249398 [details] Patch Attachment 249398 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5707102806867968 Number of test failures exceeded the failure limit.
Created attachment 249400 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249558 [details] Patch
Comment on attachment 249558 [details] Patch Attachment 249558 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5466378211426304 New failing tests: http/tests/security/frame-loading-via-document-write.html http/tests/misc/image-blocked-src-change.html http/tests/security/contentSecurityPolicy/report-blocked-file-uri.html http/tests/security/isolatedWorld/bypass-main-world-csp.html svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg http/tests/misc/image-blocked-src-no-change.html
Created attachment 249560 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249558 [details] Patch Attachment 249558 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5647604356481024 New failing tests: http/tests/security/frame-loading-via-document-write.html http/tests/misc/image-blocked-src-change.html http/tests/security/contentSecurityPolicy/report-blocked-file-uri.html http/tests/security/local-video-poster-from-remote.html http/tests/security/isolatedWorld/bypass-main-world-csp.html http/tests/security/local-image-from-remote.html svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg http/tests/misc/image-blocked-src-no-change.html
Created attachment 249561 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249716 [details] Patch
Comment on attachment 249716 [details] Patch Attachment 249716 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6447122182307840 New failing tests: svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
Created attachment 249717 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249716 [details] Patch Attachment 249716 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6144721051189248 New failing tests: svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
Created attachment 249718 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 249722 [details] Patch
Created attachment 249723 [details] Patch
Comment on attachment 249723 [details] Patch Attachment 249723 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5804133902712832 New failing tests: http/tests/security/local-video-poster-from-remote.html
Created attachment 249724 [details] Archive of layout-test-results from ews102 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249723 [details] Patch Attachment 249723 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5998535396818944 New failing tests: http/tests/security/local-video-poster-from-remote.html
Created attachment 249725 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249755 [details] Patch
Finally all green! A review would be appreciated :)
Comment on attachment 249755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249755&action=review > Source/WebCore/html/HTMLImageLoader.cpp:81 > + if (!cachedImage) > + return; > + ASSERT(cachedImage); I really hope that ASSERT can never fire :) > Source/WebCore/loader/ImageLoader.cpp:200 > + // set url value only if srcURI is not empty. Otherwise, url will be the URL for the document itself. Nit: "Set" not "set"
Comment on attachment 249755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249755&action=review > Source/WebCore/loader/ImageLoader.cpp:72 > + static std::unique_ptr<ImageLoaderTask> create(WeakPtr<ImageLoader> loader, bool shouldBypassMainWorldContentSecurityPolicy) nit: I think we decided to get rid of these factory methods for std::unique_ptr and use std::make_unique<>() at call sites. Note that it requires making the constructor public though. > Source/WebCore/loader/ImageLoader.h:80 > + bool pendingTask() const { return m_pendingTask; } for booleans, we prefer names that start with "is" or "has", in this case, hasPendingTask() / m_hasPendingTask would be nicer. > Source/WebCore/loader/ImageLoader.h:109 > + bool shouldLoadImmediately(const AtomicString&) const; I don't think we should omit the argument name here as it is not obvious what it is.
Comment on attachment 249755 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249755&action=review > Source/WebCore/loader/ImageLoader.cpp:77 > + virtual void run() Missing override. Does this really need to be public? > Source/WebCore/loader/ImageLoader.cpp:196 > + AtomicString attr = m_element.imageSourceURL(); attr is really not a great variable name. > Source/WebCore/loader/ImageLoader.cpp:301 > + AtomicString attr = m_element.imageSourceURL(); attr is not a great variable name. > Source/WebCore/loader/ImageLoader.cpp:319 > + doUpdateFromElement(false); Boolean arguments like this are really not readable. We usually use enum classes to improve readability: enum class ShouldBypassMainWorldContentSecurityPolicy { No, Yes }; doUpdateFromElement(ShouldBypassMainWorldContentSecurityPolicy::No);
Created attachment 249902 [details] Patch
Comment on attachment 249902 [details] Patch Clearing flags on attachment: 249902 Committed r182247: <http://trac.webkit.org/changeset/182247>
All reviewed patches have been landed. Closing bug.
This has broken the following test: fast/events/mouse-cursor-image-set.html
I've filed https://bugs.webkit.org/show_bug.cgi?id=143323
There are more tests broken: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2Flocal-image-from-remote.html and possibly this one, although it's flaky: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2Fframe-loading-via-document-write.html
I think we'll have to roll it out.
Re-opened since this is blocked by bug 143326
Also these CSP tests were broken on Windows: https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Freport-uri-from-javascript.html https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Freport-uri-from-inline-javascript.html
Can we revive this patch? Is there an issue that we should be blocking this bug on?
I think it'd be best to wait for https://bugs.webkit.org/show_bug.cgi?id=147933 to land and then base this patch on that microtask abstraction (which AFAIU should be more correct than it currently is)
(In reply to comment #71) > I think it'd be best to wait for > https://bugs.webkit.org/show_bug.cgi?id=147933 to land and then base this > patch on that microtask abstraction (which AFAIU should be more correct than > it currently is) It's landed now!
Cool! I'll rebase this patch to see where we're at
<rdar://problem/23754810>
Created attachment 266607 [details] Patch
Comment on attachment 266607 [details] Patch Attachment 266607 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/514521 Number of test failures exceeded the failure limit.
Created attachment 266609 [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
Comment on attachment 266607 [details] Patch Attachment 266607 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/514536 Number of test failures exceeded the failure limit.
Created attachment 266610 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 266607 [details] Patch Attachment 266607 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/514537 Number of test failures exceeded the failure limit.
Created attachment 266611 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 266613 [details] Patch
Comment on attachment 266613 [details] Patch Attachment 266613 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/514968 New failing tests: fast/dom/MutationObserver/added-out-of-order.html http/tests/misc/image-blocked-src-change.html fast/dom/MutationObserver/parser-mutations.html js/dom/Promise-static-resolve.html webarchive/loading/test-loading-archive-subresource-null-mimetype.html fast/dom/MutationObserver/removed-out-of-order.html fast/dom/MutationObserver/takeRecords.html webarchive/loading/cache-expired-subresource.html http/tests/misc/image-blocked-src-no-change.html
Created attachment 266616 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 266613 [details] Patch Attachment 266613 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/514971 New failing tests: http/tests/contentextensions/domain-rules.html http/tests/misc/image-blocked-src-change.html fast/dom/MutationObserver/parser-mutations.html http/tests/contentextensions/block-image-load-in-onunload.html fast/mediastream/MediaDevices-getUserMedia.html http/tests/contentextensions/basic-filter.html fast/dom/MutationObserver/added-out-of-order.html http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html http/tests/misc/image-blocked-src-no-change.html http/tests/contentextensions/filters-with-quantifiers-combined.html http/tests/contentextensions/block-csp-report.html fast/mediastream/getusermedia.html fast/dom/MutationObserver/removed-out-of-order.html fast/dom/MutationObserver/takeRecords.html webarchive/loading/cache-expired-subresource.html webarchive/loading/test-loading-archive-subresource-null-mimetype.html js/dom/Promise-static-resolve.html
Created attachment 266617 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 266613 [details] Patch Attachment 266613 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/514976 New failing tests: fast/workers/worker-timeout.html http/tests/misc/image-blocked-src-change.html fast/dom/MutationObserver/parser-mutations.html imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-overrides.html fast/workers/worker-strict.html fast/dom/MutationObserver/added-out-of-order.html http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html http/tests/misc/image-blocked-src-no-change.html fast/dom/MutationObserver/takeRecords.html webarchive/loading/test-loading-archive-subresource-null-mimetype.html imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-simple.html fast/dom/MutationObserver/removed-out-of-order.html imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-twice.html imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-synconworker.html webarchive/loading/cache-expired-subresource.html http/tests/websocket/tests/hybi/workers/no-onmessage-in-sync-op.html js/dom/Promise-static-resolve.html imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-aborted.html imported/w3c/web-platform-tests/XMLHttpRequest/xmlhttprequest-timeout-worker-overridesexpires.html
Created attachment 266618 [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
Comment on attachment 266613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266613&action=review > Source/WebCore/loader/ImageLoader.cpp:70 > +class ImageLoader::ImageLoaderTask : public Microtask { Should be marked final. > Source/WebCore/loader/ImageLoader.cpp:72 > + ImageLoaderTask(WeakPtr<ImageLoader> loader, CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy) This should take a WeakPtr&& and use WTF::move, and we should add a move constructor and move assignment operator to WeakPtr. > Source/WebCore/loader/ImageLoader.cpp:192 > AtomicString attr = element().imageSourceURL(); I don’t see any reason to put this into a local variable now that we are only calling a single function on it. > Source/WebCore/loader/ImageLoader.cpp:193 > + String srcURI = sourceURI(attr); I would name this local variable sourceURLString. In fact, I think the virtual function sourceURI should be renamed to sourceURLString in the future. Then this line of code would be: String sourceURLString = this->sourceURLString(element().imageSourceURL()); > Source/WebCore/loader/ImageLoader.cpp:194 > + URL url; I would name this local variable sourceURL. > Source/WebCore/loader/ImageLoader.cpp:201 > CachedResourceHandle<CachedImage> newImage = nullptr; No need for the "= nullptr" here. > Source/WebCore/loader/ImageLoader.cpp:210 > + AtomicString crossOriginMode = m_element.fastGetAttribute(HTMLNames::crossoriginAttr); The type here should be auto& or const AtomicString&; there is no need to do a bit of reference count churn on the attribute value just to call equalIgnoringCase on it. > Source/WebCore/loader/ImageLoader.cpp:211 > if (!crossOriginMode.isNull()) { Is behavior here correct? crossorigin="" means do not allow stored credentials? Do we have test cases covering this? > Source/WebCore/loader/ImageLoader.cpp:312 > + CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy shouldBypassMainWorldContentSecurityPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No; It would be better to use auto here so we didn’t have such a breathtakingly long line of code: auto shouldBypassMainWorldContentSecurityPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No; Also, for a local variable with small scope like this, I think we can use a bit of shorthand: auto shouldBypassPolicy = CachedResourceLoader::ShouldBypassMainWorldContentSecurityPolicy::No > Source/WebCore/loader/ImageLoader.cpp:326 > +bool ImageLoader::shouldLoadImmediately(const AtomicString& attribute) const This function needs a why comment explaining its policies. Why are each of these valid reasons we should load immediately? I’m not even sure these are all correct or all tested. > Source/WebCore/loader/ImageLoader.cpp:328 > + String srcURI = sourceURI(attribute); Please don’t use the name URI for any new code. I would call this sourceURLString. Also, I think early return would make this easier to read. auto sourceURLString = sourceURI(attribute); if (sourceURLString.isEmpty()) return true; > Source/WebCore/loader/ImageLoader.cpp:329 > + URL url = element().document().completeURL(srcURI); I would call this sourceURL. > Source/WebCore/loader/ImageLoader.cpp:330 > + ResourceRequest resourceRequest(url); I suggest constructing this only where it’s used and not putting it into a local variable. > Source/WebCore/loader/ImageLoader.cpp:331 > + return (srcURI.isEmpty() No need for the parentheses here. > Source/WebCore/loader/ImageLoader.cpp:333 > + || m_loadManually This should be an early exit before we do any other work. > Source/WebCore/loader/ImageLoader.cpp:334 > + || !is<HTMLImageElement>(m_element) This should be an early exit before we do work on the attribute value. > Source/WebCore/loader/ImageLoader.cpp:336 > + || MemoryCache::singleton().resourceForRequest(resourceRequest, element().document().page()->sessionID())); What guarantees page() is non-null? > Source/WebCore/loader/ImageLoader.cpp:349 > + ASSERT(resource); > ASSERT(resource == m_image.get()); Not sure I understand the need for a separate assertion here. Further, we should simply change notifyFinished to take a CachedResource& instead of a CachedResource* in the future. There’s no chance this is going to be null.
ping?
Created attachment 376850 [details] Patch
Comment on attachment 376850 [details] Patch Attachment 376850 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12950660 New failing tests: fast/images/decode-animated-image.html fast/dom/MutationObserver/parser-mutations.html fast/dom/Node/textContent-mutationEvents.html http/tests/images/decode-slow-load-static-image.html fast/images/decode-decoding-attribute-async-large-image.html js/dom/Promise-static-resolve.html http/tests/images/loading-image-no-border.html fast/shadow-dom/signal-slot-list-retains-js-wrappers.html fast/dom/MutationObserver/takeRecords.html fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html fast/events/popup-blocked-from-untrusted-mouse-click.html
Created attachment 376855 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 376864 [details] Patch
Comment on attachment 376864 [details] Patch Attachment 376864 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12951479 New failing tests: fast/images/decode-animated-image.html http/tests/images/decode-slow-load-static-image.html fast/images/decode-decoding-attribute-async-large-image.html http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html http/tests/images/loading-image-no-border.html fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html fast/events/popup-blocked-from-untrusted-mouse-click.html
Created attachment 376875 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 376896 [details] Patch
Comment on attachment 376896 [details] Patch Attachment 376896 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12952662 New failing tests: fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html http/tests/images/loading-image-no-border.html fast/events/popup-blocked-from-untrusted-mouse-click.html http/tests/security/isolatedWorld/image-load-should-not-bypass-main-world-csp.html
Created attachment 376910 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 376993 [details] Patch
Comment on attachment 376993 [details] Patch Attachment 376993 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12955667 New failing tests: fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html http/tests/images/loading-image-no-border.html
Created attachment 376995 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 377027 [details] Patch
Created attachment 377036 [details] Patch
Comment on attachment 377036 [details] Patch Attachment 377036 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12958158 New failing tests: fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html http/tests/images/loading-image-no-border.html
Created attachment 377064 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 377110 [details] Patch
Comment on attachment 377110 [details] Patch Attachment 377110 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12960141 New failing tests: fast/dom/HTMLImageElement/sizes/image-sizes-js-change-reverse.html http/tests/images/loading-image-no-border.html
Created attachment 377117 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit