Created attachment 169333 [details] Test file 1 / 2

Created attachment 169334 [details] Test file 2 / 2

If you open in the window an svg image first, then load a document, containing this image in the same window, the image will display data:uri picture.

Created attachment 233895 [details] Patch

(In reply to comment #4) > Created an attachment (id=233895) [details] > Patch Does the patch respect the fact that no further resources are allowed to be fetched from the dataURI in SVG images? No CSS, no scripts, no images, no <use> reference to external document, <iframe>, -webkit-mask, -webkit-filter and so on?

Comment on attachment 233895 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233895&action=review r- to make sure that this does not just get landed as is. > Source/WebCore/ChangeLog:7 > + in a SVG images was not loaded in most cases. On purpose! > Source/WebCore/ChangeLog:11 > + This patch enables auto-loading of images. This only affects data > + URLs since the dummy chrome and page of SVG images still do not > + have a NetworkingContext. Again, on purpose! No network requests at all from content of SVG images... dataURLs should be interpreted but the same restrictions apply here as well of course. > Source/WebCore/svg/graphics/SVGImage.cpp:367 > + m_page->settings()->setLoadsImagesAutomatically(true); Hm, in theory this allows loading further resources. Is that blocked at a different place? Could you point out where? Please also add a comment that we need to be very careful that this does not do any network requests. > LayoutTests/svg/in-html/resources/embedded.svg:3 > + <image x="20" y="20" width="30px" height="30px" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/> Please add examples with an SVG that loads another image decoded with dataURL.

(In reply to comment #6) > (From update of attachment 233895 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=233895&action=review > > r- to make sure that this does not just get landed as is. > > > Source/WebCore/ChangeLog:7 > > + in a SVG images was not loaded in most cases. > > On purpose! > On purpose that they CAN be loaded but just happens not to be unless something else triggers them to load? Currently the SVGImage allows images, but just doesn't trigger the load on its own. If the same SVG or something else with the same resource loads the URL, then the SVGImage will also show the image. We should either autoload the images that are allowed by canRequest and canDisplay logic, or we should disallow images completely. > > Source/WebCore/ChangeLog:11 > > + This patch enables auto-loading of images. This only affects data > > + URLs since the dummy chrome and page of SVG images still do not > > + have a NetworkingContext. > > Again, on purpose! No network requests at all from content of SVG images... dataURLs should be interpreted but the same restrictions apply here as well of course. I know, I wasn't complaining about that. This is just why the patch is safe, there are not NetworkingContext which means no network resources can be loaded. > > > Source/WebCore/svg/graphics/SVGImage.cpp:367 > > + m_page->settings()->setLoadsImagesAutomatically(true); > > Hm, in theory this allows loading further resources. Is that blocked at a different place? Could you point out where? Please also add a comment that we need to be very careful that this does not do any network requests. It is blocked because a) there is no networking context and b) canRequest/canDisplay would not allow it. > > > LayoutTests/svg/in-html/resources/embedded.svg:3 > > + <image x="20" y="20" width="30px" height="30px" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/> > > Please add examples with an SVG that loads another image decoded with dataURL. I am not sure I follow.

(In reply to comment #6) > > LayoutTests/svg/in-html/resources/embedded.svg:3 > > + <image x="20" y="20" width="30px" height="30px" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/> > > Please add examples with an SVG that loads another image decoded with dataURL. <img src="image.svg"> Where image.svg has an element <image xlink:href="data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10'><image xlink:href='image.png' width='10' height='10'/></svg>"/>

http://www.svachon.com/webframes/examples.html The non-vector examples do not work in WebKit/Safari, but they work everywhere else. Please fix this in time for Safari 8's release.

Hello? Does anyone give a fuck?

(In reply to comment #8) > (In reply to comment #6) > > > LayoutTests/svg/in-html/resources/embedded.svg:3 > > > + <image x="20" y="20" width="30px" height="30px" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/> > > > > Please add examples with an SVG that loads another image decoded with dataURL. > > <img src="image.svg"> > > Where image.svg has an element <image xlink:href="data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10'><image xlink:href='image.png' width='10' height='10'/></svg>"/> That wouldn't work. It can only load dataurl images.

Created attachment 236182 [details] Patch

Comment on attachment 236182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236182&action=review > Source/WebCore/ChangeLog:7 > + in a SVG images was not loaded in most cases. What does "most cases" mean? When did it work? > Source/WebCore/ChangeLog:11 > + URLs since the dummy chrome and page of SVG images still do not > + have a NetworkingContext. I would like to have that phrased differently. Otherwise someone believes that we will allow SVGImage to fetch further image resources... which we don't. > Source/WebCore/svg/graphics/SVGImage.cpp:367 > + m_page->settings().setLoadsImagesAutomatically(true); Hm, I would like to see a safety mechanism that ALWAYS disallows real image fetches. We should never ever process URLs that are something different than dataURLs. This would probably include a mechanism that actively avoids the dataURL document (assuming it is an SVG image itself) to ever load further resources. We might have that in the loader, but I am not sure. Could you check if we have this in place? And if yes, if it is strong enough? Please add a comment of your finding to the bug report. I fear that some one "fixes" NetworkingContext "issue" and suddenly fetching of further resources is possible. SVGImage must not fetch resources. > LayoutTests/ChangeLog:17 > + * svg/in-html/resources/embedded.svg: Added. I would like to see more tests 1) HTML document that loads an SVG image (like nested-data-url.html). The SVG image's <image> element references a *red* PNG. 2) HTML document that loads an SVG image (like nested-data-url.html). The SVG image's <image> element has a dataURL of an SVG image with a green square. 3) HTML document that loads an SVG image (like nested-data-url.html). The SVG image's <image> element has a dataURL of an SVG image which itself contains an <image> element that references a *red* PNG.

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 233895 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=233895&action=review > > > > r- to make sure that this does not just get landed as is. > > > > > Source/WebCore/ChangeLog:7 > > > + in a SVG images was not loaded in most cases. > > > > On purpose! > > > > On purpose that they CAN be loaded but just happens not to be unless something else triggers them to load? Currently the SVGImage allows images, but just doesn't trigger the load on its own. If the same SVG or something else with the same resource loads the URL, then the SVGImage will also show the image. Wow! That should not happen at all! SVGImage should not display the image and definitely not load the image from an SVGImage. Both cases are security concerns. > > We should either autoload the images that are allowed by canRequest and canDisplay logic, or we should disallow images completely. With the exceptions of Blobs and dataURLs they should be disabled completely. dataURLs are not allowed to trigger resource fetching and are not allowed to draw images that required resource fetching. > > > > Source/WebCore/ChangeLog:11 > > > + This patch enables auto-loading of images. This only affects data > > > + URLs since the dummy chrome and page of SVG images still do not > > > + have a NetworkingContext. > > > > Again, on purpose! No network requests at all from content of SVG images... dataURLs should be interpreted but the same restrictions apply here as well of course. > > I know, I wasn't complaining about that. This is just why the patch is safe, there are not NetworkingContext which means no network resources can be loaded. See review from patch above. What makes sure that we still won't fetch an image if someone fixes the NetworkingContext "issue"? > > > > > > Source/WebCore/svg/graphics/SVGImage.cpp:367 > > > + m_page->settings()->setLoadsImagesAutomatically(true); > > > > Hm, in theory this allows loading further resources. Is that blocked at a different place? Could you point out where? Please also add a comment that we need to be very careful that this does not do any network requests. > > It is blocked because a) there is no networking context and b) canRequest/canDisplay would not allow it. The first part would satisfy my fetching concerns. Didn't you say that we still display resources when they were fetched by something else? So canDisplay doesn't seem to work? > > > > > > LayoutTests/svg/in-html/resources/embedded.svg:3 > > > + <image x="20" y="20" width="30px" height="30px" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg=="/> > > > > Please add examples with an SVG that loads another image decoded with dataURL. > > I am not sure I follow. I hope it gets clear with the review.

(In reply to comment #13) > (From update of attachment 236182 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236182&action=review > > > Source/WebCore/ChangeLog:7 > > + in a SVG images was not loaded in most cases. > > What does "most cases" mean? When did it work? > The data-url images are allowed, but not loaded. If something else loads the same data-url the SVG will show it. > > Source/WebCore/ChangeLog:11 > > + URLs since the dummy chrome and page of SVG images still do not > > + have a NetworkingContext. > > I would like to have that phrased differently. Otherwise someone believes that we will allow SVGImage to fetch further image resources... which we don't. > Okay. > > Source/WebCore/svg/graphics/SVGImage.cpp:367 > > + m_page->settings().setLoadsImagesAutomatically(true); > > Hm, I would like to see a safety mechanism that ALWAYS disallows real image fetches. We should never ever process URLs that are something different than dataURLs. This would probably include a mechanism that actively avoids the dataURL document (assuming it is an SVG image itself) to ever load further resources. We might have that in the loader, but I am not sure. Could you check if we have this in place? And if yes, if it is strong enough? Please add a comment of your finding to the bug report. We already have mechanisms for that that check our policies and enforce them. This patch only deals with the case of images that our policy checked allowed but just wasn't loaded. It is a few months since I digged through this, but as remember it there are two important checks in SecurityOrigin canRequest and canDisplay. In this case of data-urls canDisplay allows it, but with blob-urls it delegates to canRequest(). Data URLs are for the checking policy also considered unique origins, so they are not allowed to load anything else.

Created attachment 236188 [details] as requested by Dirk Schulze nested-data-url.html green-embedded-embedded.svg green-embedded.svg green-referenced-embedded.svg green-referenced.svg green.svg red-embedded-embedded.svg red-embedded.svg red-referenced-embedded.svg red-referenced.svg red.png

https://bugs.webkit.org/attachment.cgi?id=236188&action=edit(In reply to comment #16) > Created an attachment (id=236188) [details] > as requested by Dirk Schulze > > nested-data-url.html > > green-embedded-embedded.svg > green-embedded.svg > green-referenced-embedded.svg > green-referenced.svg > green.svg > red-embedded-embedded.svg > red-embedded.svg > red-referenced-embedded.svg > red-referenced.svg > red.png Oh, not two colors per test case... the colors should be an indicator... if you see red, the test doesn't pass. That is why the SVGImage with external references should all embed red pictures.... in cases where it is ok (dataURL without references) it should be green. So green-referenced-embedded.svg is not correct for instance. Please add the tests to the patch. They look good to me.

I'd thought you wanted green to distinguish vector from raster. Would you like the tests updated?

(In reply to comment #18) > I'd thought you wanted green to distinguish vector from raster. Would you like the tests updated? The color red is used as an indicator: If you see red it means something went wrong. So the tests where the image should NOT load and/or display, the referenced image should be red. Where you DO want to see the image (because it is a datURL without further resource fetches) it should be green or at least not red.

Created attachment 236204 [details] as requested by Dirk Schulze (In reply to comment #19) > The color red is used as an indicator: If you see red it means something went wrong. So the tests where the image should NOT load and/or display, the referenced image should be red. Where you DO want to see the image (because it is a datURL without further resource fetches) it should be green or at least not red. Updated. You guys will have to add it to the patch, though, as I do not write C.

Created attachment 236278 [details] Patch

Comment on attachment 236278 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236278&action=review Nearly there. Just some more updates to the tests. > LayoutTests/ChangeLog:21 > + * svg/in-html/nested-data-url.html: Added. I didn't see a reference file so that we run reference tests. Could you please add one? > LayoutTests/svg/in-html/resources/raster-referenced-embedded.svg:3 > +<image width="100" height="100" xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgdmlld0JveD0iMCAwIDEwMCAxMDAiIHByZXNlcnZlQXNwZWN0UmF0aW89Im5vbmUiPgoKPGltYWdlIHdpZHRoPSIxMDAiIGhlaWdodD0iMTAwIiB4bGluazpocmVmPSJyYXN0ZXIucG5nIi8+Cgo8L3N2Zz4="/> This should be an red image that is referenced. > LayoutTests/svg/in-html/resources/raster-referenced.svg:3 > +<image width="100" height="100" xlink:href="raster.png"/> This should reference a red image. Loading would be an error. > LayoutTests/svg/in-html/resources/vector-referenced-embedded.svg:3 > +<image width="100" height="100" xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgdmlld0JveD0iMCAwIDEwMCAxMDAiIHByZXNlcnZlQXNwZWN0UmF0aW89Im5vbmUiPgoKPGltYWdlIHdpZHRoPSIxMDAiIGhlaWdodD0iMTAwIiB4bGluazpocmVmPSJ2ZWN0b3Iuc3ZnIi8+Cgo8L3N2Zz4="/> This should be a reference to a red image. Because it would be an error if it is displayed. > LayoutTests/svg/in-html/resources/vector-referenced.svg:3 > +<image width="100" height="100" xlink:href="vector.svg"/> vector.svg should be a red image. If it loads, then this is a bug.

(In reply to comment #22) > (From update of attachment 236278 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236278&action=review > > Nearly there. Just some more updates to the tests. > > > LayoutTests/ChangeLog:21 > > + * svg/in-html/nested-data-url.html: Added. > > I didn't see a reference file so that we run reference tests. Could you please add one? > > > LayoutTests/svg/in-html/resources/raster-referenced-embedded.svg:3 > > +<image width="100" height="100" xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgdmlld0JveD0iMCAwIDEwMCAxMDAiIHByZXNlcnZlQXNwZWN0UmF0aW89Im5vbmUiPgoKPGltYWdlIHdpZHRoPSIxMDAiIGhlaWdodD0iMTAwIiB4bGluazpocmVmPSJyYXN0ZXIucG5nIi8+Cgo8L3N2Zz4="/> > > This should be an red image that is referenced. > > > LayoutTests/svg/in-html/resources/raster-referenced.svg:3 > > +<image width="100" height="100" xlink:href="raster.png"/> > > This should reference a red image. Loading would be an error. > > > LayoutTests/svg/in-html/resources/vector-referenced-embedded.svg:3 > > +<image width="100" height="100" xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgdmlld0JveD0iMCAwIDEwMCAxMDAiIHByZXNlcnZlQXNwZWN0UmF0aW89Im5vbmUiPgoKPGltYWdlIHdpZHRoPSIxMDAiIGhlaWdodD0iMTAwIiB4bGluazpocmVmPSJ2ZWN0b3Iuc3ZnIi8+Cgo8L3N2Zz4="/> > > This should be a reference to a red image. Because it would be an error if it is displayed. > > > LayoutTests/svg/in-html/resources/vector-referenced.svg:3 > > +<image width="100" height="100" xlink:href="vector.svg"/> > > vector.svg should be a red image. If it loads, then this is a bug. Yes, half them should be red. I left them in so that are also testing that they can't load further images. I will see what I can do about the reference.

(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 236278 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=236278&action=review > > > > Nearly there. Just some more updates to the tests. > > > > > LayoutTests/ChangeLog:21 > > > + * svg/in-html/nested-data-url.html: Added. > > > > I didn't see a reference file so that we run reference tests. Could you please add one? > > > > > LayoutTests/svg/in-html/resources/raster-referenced-embedded.svg:3 > > > +<image width="100" height="100" xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgdmlld0JveD0iMCAwIDEwMCAxMDAiIHByZXNlcnZlQXNwZWN0UmF0aW89Im5vbmUiPgoKPGltYWdlIHdpZHRoPSIxMDAiIGhlaWdodD0iMTAwIiB4bGluazpocmVmPSJyYXN0ZXIucG5nIi8+Cgo8L3N2Zz4="/> > > > > This should be an red image that is referenced. > > > > > LayoutTests/svg/in-html/resources/raster-referenced.svg:3 > > > +<image width="100" height="100" xlink:href="raster.png"/> > > > > This should reference a red image. Loading would be an error. > > > > > LayoutTests/svg/in-html/resources/vector-referenced-embedded.svg:3 > > > +<image width="100" height="100" xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIiB3aWR0aD0iMTAwIiBoZWlnaHQ9IjEwMCIgdmlld0JveD0iMCAwIDEwMCAxMDAiIHByZXNlcnZlQXNwZWN0UmF0aW89Im5vbmUiPgoKPGltYWdlIHdpZHRoPSIxMDAiIGhlaWdodD0iMTAwIiB4bGluazpocmVmPSJ2ZWN0b3Iuc3ZnIi8+Cgo8L3N2Zz4="/> > > > > This should be a reference to a red image. Because it would be an error if it is displayed. > > > > > LayoutTests/svg/in-html/resources/vector-referenced.svg:3 > > > +<image width="100" height="100" xlink:href="vector.svg"/> > > > > vector.svg should be a red image. If it loads, then this is a bug. > > Yes, half them should be red. I left them in so that are also testing that they can't load further images. Pleases let the tests in! They are great. There should just be red images for the cases where we do not expect image loading. > > I will see what I can do about the reference. Thanks!

(In reply to comment #24) > Pleases let the tests in! They are great. There should just be red images for the cases where we do not expect image loading. > I think the red background-color showing where an image is lacking is enough. I just have to make sure it doesn't show the 'missing image' image.

Created attachment 236281 [details] Patch

I'm updating the tests. They'll be ready soon. Also adding "direct" tests involving iframes.

Comment on attachment 236281 [details] Patch Attachment 236281 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6110123193794560 New failing tests: svg/in-html/nested-data-url.html

Created attachment 236285 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Created attachment 236286 [details] as requested by Dirk Schulze iframe-svg-image-dataurl/ iframe-svg-image-reference/ img-svg-image-dataurl/ img-svg-image-reference/ stylesheet.css

Comment on attachment 236281 [details] Patch Attachment 236281 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6345089848705024 New failing tests: svg/in-html/nested-data-url.html

Created attachment 236290 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 236281 [details] Patch Attachment 236281 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5092397138575360 New failing tests: svg/in-html/nested-data-url.html

Created attachment 236300 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Please add (id=236286)[#236286] to the patch.

Comment on attachment 236281 [details] Patch Attachment 236281 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5712084683718656 New failing tests: svg/in-html/nested-data-url.html

Created attachment 236303 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 236281 [details] Patch svg/in-html/nested-data-url.html is failing on the two Mac EWS bots

(In reply to comment #38) > (From update of attachment 236281 [details]) > svg/in-html/nested-data-url.html is failing on the two Mac EWS bots Yes I noticed, and the failure looks like the data-url image is not loaded at all, but my original reference test in the 7th of August patch did pass on the bots.

Add the latest tests to the patch, please. They're in https://bugs.webkit.org/attachment.cgi?id=236286

I guess that no one cares about this bug.

*** Bug 137941 has been marked as a duplicate of this bug. ***

*** Bug 118068 has been marked as a duplicate of this bug. ***

The bug is not repro anymore. We explicitly disallow loading external resources loaded by an SVG referenced in an <img> tag expect for data uri. Please see https://bugs.webkit.org/show_bug.cgi?id=137762.

"You are not authorized to access bug #137762." ?

I was wrong. The bug is still repro but the image should not be in the cache. I loaded the svg first which puts in the cache. Then when I loaded the page, the image was in the cache so it was displaying fine. So I am reopening the bug.

(In reply to comment #45) > "You are not authorized to access bug #137762." > ? Because this a security bug. You have to ask some one from the security team to give you access to it.

Created attachment 243522 [details] Patch

I think the original patch was incorrect. I think there was also a confusion about whether this patch fixed the bug or not. If the svg image was loaded as a standalone file before loading the the html, the bug does not happen. And this might explains why the test in the patch was not passing in Bot.

Comment on attachment 243522 [details] Patch Attachment 243522 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4606574513356800 New failing tests: fast/forms/basic-buttons.html compositing/tiling/huge-layer-img.html

Created attachment 243525 [details] Archive of layout-test-results from ews105 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 243522 [details] Patch Attachment 243522 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6210498911535104 New failing tests: fast/forms/basic-buttons.html compositing/tiling/huge-layer-img.html

Created attachment 243527 [details] Archive of layout-test-results from ews100 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 243522 [details] Patch Looking at the failures.

Created attachment 243601 [details] Patch

Comment on attachment 243601 [details] Patch Attachment 243601 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4527628182618112 New failing tests: fast/forms/basic-buttons.html

Created attachment 243602 [details] Archive of layout-test-results from ews105 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 243601 [details] Patch Attachment 243601 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6128451614408704 New failing tests: fast/forms/basic-buttons.html

Created attachment 243603 [details] Archive of layout-test-results from ews101 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Created attachment 243605 [details] Patch

Comment on attachment 243605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243605&action=review > Source/WebCore/ChangeLog:8 > + Loading data URI SVG resources should bypass the security checks and should happen The phrase "bypass the security checks" doesn't sounds good. Can you elaborate on the security checks that prevented the load of a data URL in an SVG document embedded in an HTML document? How does making data URL loading synchronous fix this SVG issue? > Source/WebCore/ChangeLog:10 > + synchronously. The workflow of loading external resources is not suitable for loading > + data URI resources. Once the data URI is set to the resource link attribute, the data Can you elaborate on why the "workflow of loading external resources is not suitable for loading data URI resources"? > Source/WebCore/ChangeLog:11 > + of the resource is parsed form the URI and set to the resource. The resource is marked Nit: form => from > Source/WebCore/ChangeLog:26 > + This is a debug code for checking recursive calls to loadPendingResources(). The code Nit: "is a" => is > Source/WebCore/css/StyleResolver.cpp:3654 > + static Vector<Document*>* vector = new Vector<Document*>(); Nit: We should use NeverDestroyed<> (*) here and have this function return a reference instead of heap allocating Vector(), as a means to prevent its destructor from being called, and returning a pointer. See the documentation at the top of Source/WTF/wtf/NeverDestroyed.h for more details, including a usage example: <http://trac.webkit.org/browser/trunk/Source/WTF/wtf/NeverDestroyed.h?rev=169518#L34>. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:135 > + // Data URI images can be processed synchronously and avoid using the resource loader. > + if (type == CachedResource::ImageResource && request.url().protocolIsData()) { > + String mimeType, charset; > + RefPtr<SharedBuffer> buffer = parseDataURL(request.url().string(), mimeType, charset); > + > + if (buffer) { > + ResourceResponse response(request.url(), mimeType, buffer->size(), charset); > + resource->responseReceived(response); > + > + // finialize the loading will set the resouce actual data > + resource->finishLoading(buffer.get()); > + resource->finish(); > + } > + } Can you elaborate on the necessity of this code given that we don't seem to have similar parsing/loading logic in WebCore for loading a data URL with respect to a HTML Image element? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:520 > + if (((policy != Use && resource->status() != CachedResource::Cached) || resource->stillNeedsLoad()) && CachedResourceRequest::NoDefer == request.defer()) { This is incorrect for non-data URLs because a server may request that a resource not be cached (via the HTTP Cache-Control header(*)) among other cache policy options. (*) "Cache Control Extensions", Hypertext Transfer Protocol -- HTTP 1.1 (RFC2616), <http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.6> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:555 > + CachedResourceHandle<CachedResource> newResource = createAndLoadResource(resource->type(), resource->resourceRequest(), resource->encoding(), resource->sessionID()); Can you elaborate on this change? Specifically, when does a data URL need to be revalidated? > Source/WebCore/platform/URL.cpp:1967 > + return extractMIMETypeFromMediaType(mediaType); This doesn't seem correct. While we may be able to get away with using extractMIMETypeFromMediaType() to extract the MIME type, its parsing rules are with respect to parsing HTTP headers, in particular the HTTP Content Type header. > Source/WebCore/platform/URL.cpp:2011 > +PassRefPtr<SharedBuffer> parseDataURL(const String& url, String& mimeType, String& charset) > +{ > + String mediaType = mediaTypeFromDataURL(url); > + if (mediaType.isEmpty()) > + return nullptr; > + > + // Skip "data:", mediaType and the seprator ",|;" at the end > + String data = url.substring(5 + mediaType.length() + 1); > + > + bool base64 = mediaType.endsWith(";base64", false); > + if (base64) > + mediaType = mediaType.left(mediaType.length() - 7); > + > + if (mediaType.isEmpty()) > + mediaType = "text/plain,"; > + > + mimeType = extractMIMETypeFromMediaType(mediaType); > + charset = extractCharsetFromMediaType(mediaType); > + > + if (charset.isEmpty()) > + charset = "US-ASCII"; > + > + if (base64) { > + data = decodeURLEscapeSequences(data); > + Vector<char> result; > + base64Decode(data, result, Base64IgnoreWhitespace); > + return SharedBuffer::create(result.data(), result.size()); > + } > + > + TextEncoding encoding(charset); > + data = decodeURLEscapeSequences(data, encoding); > + return SharedBuffer::create(data.characters8(), data.length()); > +} I haven't looked over this code in detail. I'll wait for your reply with regards to whether we need such logic in WebCore. If so, then I'll review this code.

Created attachment 244011 [details] Patch

(In reply to comment #61) > Comment on attachment 243605 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243605&action=review > > > Source/WebCore/ChangeLog:8 > > + Loading data URI SVG resources should bypass the security checks and should happen > > The phrase "bypass the security checks" doesn't sounds good. Can you > elaborate on the security checks that prevented the load of a data URL in an > SVG document embedded in an HTML document? How does making data URL loading > synchronous fix this SVG issue? > Fixed. Loading resources expect for top-level documents is prevented. For example, an SVG document, which is created from an <img> tag, can't load any sub-resource. For data uri, the image is already loaded. All we need to do is to decoded the text and create the resource data object. > > Source/WebCore/ChangeLog:10 > > + synchronously. The workflow of loading external resources is not suitable for loading > > + data URI resources. Once the data URI is set to the resource link attribute, the data > > Can you elaborate on why the "workflow of loading external resources is not > suitable for loading data URI resources"? > Answered above. > > Source/WebCore/ChangeLog:11 > > + of the resource is parsed form the URI and set to the resource. The resource is marked > > Nit: form => from > Done. > > Source/WebCore/ChangeLog:26 > > + This is a debug code for checking recursive calls to loadPendingResources(). The code > > Nit: "is a" => is > Done. > > Source/WebCore/css/StyleResolver.cpp:3654 > > + static Vector<Document*>* vector = new Vector<Document*>(); > > Nit: We should use NeverDestroyed<> (*) here and have this function return a > reference instead of heap allocating Vector(), as a means to prevent its > destructor from being called, and returning a pointer. See the documentation > at the top of Source/WTF/wtf/NeverDestroyed.h for more details, including a > usage example: > <http://trac.webkit.org/browser/trunk/Source/WTF/wtf/NeverDestroyed. > h?rev=169518#L34>. > Done. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:135 > > + // Data URI images can be processed synchronously and avoid using the resource loader. > > + if (type == CachedResource::ImageResource && request.url().protocolIsData()) { > > + String mimeType, charset; > > + RefPtr<SharedBuffer> buffer = parseDataURL(request.url().string(), mimeType, charset); > > + > > + if (buffer) { > > + ResourceResponse response(request.url(), mimeType, buffer->size(), charset); > > + resource->responseReceived(response); > > + > > + // finialize the loading will set the resouce actual data > > + resource->finishLoading(buffer.get()); > > + resource->finish(); > > + } > > + } > > Can you elaborate on the necessity of this code given that we don't seem to > have similar parsing/loading logic in WebCore for loading a data URL with > respect to a HTML Image element? > I believe loading the data uri synchronously is the right way because, actually we do not load any data. All we need to do is decoding the text and creating the resource data object. To me, it does not make sense to change all the sub-resource loading workflow and security checks and go though a different thread and wait for the resource-loaded notification just because we do not want to do the decoding. If the data uri image is an SVG, extra steps will take place synchronously, like creating an SVG document and resolving its css before finishing loading the top-level document. But this scenario seems to work fine. Also the image onload() event fires only after loading the main document. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:520 > > + if (((policy != Use && resource->status() != CachedResource::Cached) || resource->stillNeedsLoad()) && CachedResourceRequest::NoDefer == request.defer()) { > > This is incorrect for non-data URLs because a server may request that a > resource not be cached (via the HTTP Cache-Control header(*)) among other > cache policy options. > > (*) "Cache Control Extensions", Hypertext Transfer Protocol -- HTTP 1.1 > (RFC2616), <http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.6> > Fixed. I wanted something to tell me if the image data resource object is created or not. I changed the code to handle this case more robustly. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:555 > > + CachedResourceHandle<CachedResource> newResource = createAndLoadResource(resource->type(), resource->resourceRequest(), resource->encoding(), resource->sessionID()); > > Can you elaborate on this change? Specifically, when does a data URL need to > be revalidated? > This change is now reverted back. And yes you are right, revalidateResource() should not be called for data uri images. > > Source/WebCore/platform/URL.cpp:1967 > > + return extractMIMETypeFromMediaType(mediaType); > > This doesn't seem correct. While we may be able to get away with using > extractMIMETypeFromMediaType() to extract the MIME type, its parsing rules > are with respect to parsing HTTP headers, in particular the HTTP Content > Type header. > The data uri scheme is the following: data:[<MIME-type>][;charset=<encoding>][;base64],<data> The mime-type is the sub-string following the "data:" and before the first sime-colon. If no-sime-colon exists, we get the sub-string before the first comma. And this what extractMIMETypeFromMediaType() does. > > Source/WebCore/platform/URL.cpp:2011 > > +PassRefPtr<SharedBuffer> parseDataURL(const String& url, String& mimeType, String& charset) > > +{ > > + String mediaType = mediaTypeFromDataURL(url); > > + if (mediaType.isEmpty()) > > + return nullptr; > > + > > + // Skip "data:", mediaType and the seprator ",|;" at the end > > + String data = url.substring(5 + mediaType.length() + 1); > > + > > + bool base64 = mediaType.endsWith(";base64", false); > > + if (base64) > > + mediaType = mediaType.left(mediaType.length() - 7); > > + > > + if (mediaType.isEmpty()) > > + mediaType = "text/plain,"; > > + > > + mimeType = extractMIMETypeFromMediaType(mediaType); > > + charset = extractCharsetFromMediaType(mediaType); > > + > > + if (charset.isEmpty()) > > + charset = "US-ASCII"; > > + > > + if (base64) { > > + data = decodeURLEscapeSequences(data); > > + Vector<char> result; > > + base64Decode(data, result, Base64IgnoreWhitespace); > > + return SharedBuffer::create(result.data(), result.size()); > > + } > > + > > + TextEncoding encoding(charset); > > + data = decodeURLEscapeSequences(data, encoding); > > + return SharedBuffer::create(data.characters8(), data.length()); > > +} > > I haven't looked over this code in detail. I'll wait for your reply with > regards to whether we need such logic in WebCore. If so, then I'll review > this code. I changed this function a little since the URL precent decoding was not working correctly.

Created attachment 244030 [details] Patch

Comment on attachment 244030 [details] Patch Attachment 244030 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6621323807686656 New failing tests: fast/forms/basic-buttons.html

Created attachment 244032 [details] Archive of layout-test-results from ews105 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 244030 [details] Patch Attachment 244030 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4848738728148992 New failing tests: fast/forms/basic-buttons.html

Created attachment 244034 [details] Archive of layout-test-results from ews100 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 244030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244030&action=review > Source/WebCore/ChangeLog:8 > + Loading data URI SVG resources should happen synchronously. The workflow of Synchronous loading is a change that is observable from JavaScript in many ways (e.g. load events). What do other browsers do? In addition to the compatibility concern, doing synchronous event dispatch is generally bad for security, because an event handler can change the DOM in ways that parser is not prepared to handle. > Source/WebCore/ChangeLog:51 > + Implement a data URI parser. It was mostly taken from platform/network/DataURL.cpp. Should DataURL use the code in URL.cpp now? I don't think that we want two copies of the code. > Source/WebCore/ChangeLog:55 > + * platform/text/TextCodecLatin1.cpp: > + (WebCore::TextCodecLatin1::decode): > + This function has a bug when it goes through the fast decoding path. After copying one or I think that it should be landed as a separate fix with a regression test of its own. > Source/WebCore/css/StyleResolver.cpp:3423 > // Re-entering this function will probably mean trouble. Catch it in debug builds. It is surprising if we now have a code path that can cause re-entering this function from a different document, but not from the same one. I don't think that this can be the case. For example, if you have a load event handler on an element, and the handler is synchronously called while parsing, then the handler can insert another element with a data url source, which will be loaded synchronously too. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:533 > + // Check if the resource can be loaded synchronously instead of using the resource loader > + if (!synchronousLoadResource(resource->resourceRequest(), request.defer(), resource.get())) The resource loader doesn't just load resources - it also implements a number of policy checks. What comes to mind immediately is that it can be in a "defersLoading" state, and that there are client calls to inform the application (and WebInspector) about the loading. There is also (theoretically) some scheduling to prioritize more important resources, although I'm not sure if that matters for data: URLs. It might matter. > Source/WebCore/platform/URL.cpp:1953 > + index = url.find(';'); Is data:text/plain;content a valid data URL? When I load '<iframe src="data:text/plain;content"></iframe>' in shipping Safari, nothing loads, which suggests to me that it's not. > Source/WebCore/platform/URL.cpp:1958 > + if (index < 5) I don't think that this can happen. The first 4 characters are "data:", there is no comma or semicolon here. > Source/WebCore/platform/URL.cpp:1959 > + return "text/plain,"; // Data URLs with no MIME type are considered text/plain. This needs to be ASCIILiteral(). Also, is this even true? When I do '<iframe src="data:aaaabbb"></iframe>' in shipping Safari, there is nothing rendered, so apparently the URL is not parsed as described here. > Source/WebCore/platform/URL.cpp:1983 > + if (mediaType.isEmpty()) > + return nullptr; I didn't find a test for this special case. Does this change behavior compared to CFNetwork data URL loading? > Source/WebCore/platform/URL.cpp:1985 > + // Skip "data:", mediaType and the seprator ",|;" at the end Typo: seprator. > Source/WebCore/platform/URL.cpp:1988 > + bool base64 = mediaType.endsWith(";base64", false); Is data:text/plain;base64;charset=utf-8,... valid? If so, endsWith is not the right check. > Source/WebCore/platform/URL.cpp:2010 > + CString encodedData = encoding.encode(data, URLEncodedEntitiesForUnencodables); This seems quite wasteful - we don't need the data encoded just to be decoded once again. > LayoutTests/ChangeLog:27 > + * platform/mac/fast/forms/basic-buttons-expected.png: > + * platform/mac/fast/forms/basic-buttons-expected.txt: > + * platform/mac-mavericks/fast/forms/basic-buttons-expected.txt: EWS fails because this patch also needs to update mac-mountainlion results. You can take them from the uploaded EWS archive. > LayoutTests/compositing/tiling/huge-layer-img.html:33 > + setTimeout(doDumpTree, 100); This is not right - a 100 ms timeout will likely be insufficient when running under GuardMalloc, ASan or leaks tool. Even on release builds, we use hyper-threading, which means that some tests run on very slow virtual cores.

Created attachment 244103 [details] Patch

Created attachment 244110 [details] Patch

(In reply to comment #69) > Comment on attachment 244030 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244030&action=review > > > Source/WebCore/ChangeLog:8 > > + Loading data URI SVG resources should happen synchronously. The workflow of > > Synchronous loading is a change that is observable from JavaScript in many > ways (e.g. load events). What do other browsers do? > > In addition to the compatibility concern, doing synchronous event dispatch > is generally bad for security, because an event handler can change the DOM > in ways that parser is not prepared to handle. > I do not think this is a concern. All the image onload events are sent only after loading the main document. I included the test svg/as-image/svg-image-with-data-uri-load-event.html to confirm that. The order of completing the resources in a page is random. It depends on the servers' response time and how big the resources are. Since the data uri images are loaded with the main document, it is expected that they should receive their onload events first. > > Source/WebCore/ChangeLog:51 > > + Implement a data URI parser. It was mostly taken from platform/network/DataURL.cpp. > > Should DataURL use the code in URL.cpp now? I don't think that we want two > copies of the code. > This file platform/network/DataURL.cpp is not included in the WebKit project > > Source/WebCore/ChangeLog:55 > > + * platform/text/TextCodecLatin1.cpp: > > + (WebCore::TextCodecLatin1::decode): > > + This function has a bug when it goes through the fast decoding path. After copying one or > > I think that it should be landed as a separate fix with a regression test of > its own. > I will sort this out once we agree on the approach to fix this bug. > > Source/WebCore/css/StyleResolver.cpp:3423 > > // Re-entering this function will probably mean trouble. Catch it in debug builds. > > It is surprising if we now have a code path that can cause re-entering this > function from a different document, but not from the same one. I don't think > that this can be the case. > > For example, if you have a load event handler on an element, and the handler > is synchronously called while parsing, then the handler can insert another > element with a data url source, which will be loaded synchronously too. > This case happen if the css section of a document has a data uri SVG image. To load this image an SVG document has to be created and its css has to be resolved while resolving the css of the main has not finished yet. I included the test svg/as-image/svg-image-with-data-uri-background.html to test this case. > > Source/WebCore/loader/cache/CachedResourceLoader.cpp:533 > > + // Check if the resource can be loaded synchronously instead of using the resource loader > > + if (!synchronousLoadResource(resource->resourceRequest(), request.defer(), resource.get())) > > The resource loader doesn't just load resources - it also implements a > number of policy checks. What comes to mind immediately is that it can be in > a "defersLoading" state, and that there are client calls to inform the > application (and WebInspector) about the loading. > > There is also (theoretically) some scheduling to prioritize more important > resources, although I'm not sure if that matters for data: URLs. It might > matter. > I think synchronousLoadResource() might be a misleading name since we do not load anything. The image is already loaded with the main document. I can change the name if this make the fix cleaner. The case for turning imagesEnabled off is covered by the checking defer != CachedResourceRequest::DeferredByClient. I added the test svg/as-image/svg-image-with-data-uri-images-disabled.html to cover this case. > > Source/WebCore/platform/URL.cpp:1953 > > + index = url.find(';'); > > Is data:text/plain;content a valid data URL? When I load '<iframe > src="data:text/plain;content"></iframe>' in shipping Safari, nothing loads, > which suggests to me that it's not. > Fixed. The data uri scheme is the following: data:[<MIME-type>][;charset=<encoding>][;base64],<data> This means it has to start with a "data:" and end with ','. > > Source/WebCore/platform/URL.cpp:1958 > > + if (index < 5) > > I don't think that this can happen. The first 4 characters are "data:", > there is no comma or semicolon here. > "data:" is 5 characters string :) > > Source/WebCore/platform/URL.cpp:1959 > > + return "text/plain,"; // Data URLs with no MIME type are considered text/plain. > > This needs to be ASCIILiteral(). Done. > > Also, is this even true? When I do '<iframe src="data:aaaabbb"></iframe>' in > shipping Safari, there is nothing rendered, so apparently the URL is not > parsed as described here. > Your data uri is wrong. It has to be <iframe src="data,:aaaabbb"></iframe>'. (I added a comma after "data:"). > > Source/WebCore/platform/URL.cpp:1983 > > + if (mediaType.isEmpty()) > > + return nullptr; > > I didn't find a test for this special case. Does this change behavior > compared to CFNetwork data URL loading? > I will add a test for this. > > Source/WebCore/platform/URL.cpp:1985 > > + // Skip "data:", mediaType and the seprator ",|;" at the end > > Typo: seprator. > Fixed. > > Source/WebCore/platform/URL.cpp:1988 > > + bool base64 = mediaType.endsWith(";base64", false); > > Is data:text/plain;base64;charset=utf-8,... valid? If so, endsWith is not > the right check. > The scheme is data:[<MIME-type>][;charset=<encoding>][;base64],<data>. So ";base64" has to come at the end of the media type. > > Source/WebCore/platform/URL.cpp:2010 > > + CString encodedData = encoding.encode(data, URLEncodedEntitiesForUnencodables); > > This seems quite wasteful - we don't need the data encoded just to be > decoded once again. > Actually it is needed. decodeURLEscapeSequences(data, encoding) decodes the URL percent text to US-ASCII. If a character is greater than 127, we encode it a double byte character. For example a string "%80" is converted to 0x20ac. And the string has to be converted to 16 bit characters. To convert the string back to single byte string encoding.encode(data, URLEncodedEntitiesForUnencodables) is used. > > LayoutTests/ChangeLog:27 > > + * platform/mac/fast/forms/basic-buttons-expected.png: > > + * platform/mac/fast/forms/basic-buttons-expected.txt: > > + * platform/mac-mavericks/fast/forms/basic-buttons-expected.txt: > > EWS fails because this patch also needs to update mac-mountainlion results. > You can take them from the uploaded EWS archive. > Done. > > LayoutTests/compositing/tiling/huge-layer-img.html:33 > > + setTimeout(doDumpTree, 100); > > This is not right - a 100 ms timeout will likely be insufficient when > running under GuardMalloc, ASan or leaks tool. Even on release builds, we > use hyper-threading, which means that some tests run on very slow virtual > cores. Done. I increased the time to be 500 ms. However with this patch, this time will not be needed since the image size will be available immediately and the decision for making the tilting will be made faster.

Comment on attachment 244110 [details] Patch Attachment 244110 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6479911472594944 New failing tests: fast/css/direct-adjacent-style-update-optimization.html

Created attachment 244119 [details] Archive of layout-test-results from ews101 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 244110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244110&action=review > Source/WebCore/ChangeLog:62 > + * platform/text/TextCodecLatin1.cpp: > + (WebCore::TextCodecLatin1::decode): > + This function has a bug when it goes through the fast decoding path. After copying one or > + more all ASCII MachineWords from source to the destination, the following byte is copied > + as is from the source to the destination even if it is non ASCII byte. This causes the decoded > + bytes to be incorrect. The fix is to use the mapping table always since it returns the same > + value if the byte is ASCII. An alternative solution is to check isASCII(*source) is true > + before setting *destination = *source. But the function call seems to be more expensive > + than the array indexing. The test imported/mozilla/svg/filters/feSpecularLighting-1.svg was > + failing after applying my patch because of this bug. The expected result is an SVG which > + contains a data uri image. Can this part of the patch be tested and landed separately?

> All the image onload events are sent only after loading the main document. Is this the correct behavior? Why? > This file platform/network/DataURL.cpp is not included in the WebKit project You are saying that we don't build it on Mac or iOS, but this is not a good reason to copy/paste code within WebCore, especially for something as important as data URL parsing. > The data uri scheme is the following: > > data:[<MIME-type>][;charset=<encoding>][;base64],<data> This is what an RFC says, but is this how it works? How did you confirm that? > Done. I increased the time to be 500 ms. However with this patch, this time will not be needed since the image size will be available immediately and the decision for making the tilting will be made faster. If the timeout is not needed, then you shouldn't add it. Nothing guarantees that 500 ms is enough either, we should never use any timeouts in layout tests other than 0 ms. > To convert the string back to single byte string encoding.encode(data, URLEncodedEntitiesForUnencodables) is used. This still appears wasteful, and we need to structure the code differently. > > > Source/WebCore/platform/URL.cpp:1958 >> > + if (index < 5) > > > > I don't think that this can happen. The first 4 characters are "data:", > > there is no comma or semicolon here. > > "data:" is 5 characters string :) My comment stands, this is dead code.

Comment on attachment 244110 [details] Patch We should probably talk through the approach in person, but this doesn't address most of my smaller comments on the previous patch, which need to be addressed. Also, please split the decoder fix into a separate bug, it needs to be discussed separately, so keeping it here doesn't help.

(In reply to comment #75) > Comment on attachment 244110 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244110&action=review > > > Source/WebCore/ChangeLog:62 > > + * platform/text/TextCodecLatin1.cpp: > > + (WebCore::TextCodecLatin1::decode): > > + This function has a bug when it goes through the fast decoding path. After copying one or > > + more all ASCII MachineWords from source to the destination, the following byte is copied > > + as is from the source to the destination even if it is non ASCII byte. This causes the decoded > > + bytes to be incorrect. The fix is to use the mapping table always since it returns the same > > + value if the byte is ASCII. An alternative solution is to check isASCII(*source) is true > > + before setting *destination = *source. But the function call seems to be more expensive > > + than the array indexing. The test imported/mozilla/svg/filters/feSpecularLighting-1.svg was > > + failing after applying my patch because of this bug. The expected result is an SVG which > > + contains a data uri image. > > Can this part of the patch be tested and landed separately? Done. https://bugs.webkit.org/show_bug.cgi?id=140173

(In reply to comment #72) > (In reply to comment #69) > > Synchronous loading is a change that is observable from JavaScript in many > > ways (e.g. load events). What do other browsers do? > > > > In addition to the compatibility concern, doing synchronous event dispatch > > is generally bad for security, because an event handler can change the DOM > > in ways that parser is not prepared to handle. > > > > I do not think this is a concern. All the image onload events are sent only > after loading the main document. I included the test > svg/as-image/svg-image-with-data-uri-load-event.html to confirm that. The > order of completing the resources in a page is random. It depends on the > servers' response time and how big the resources are. Since the data uri > images are loaded with the main document, it is expected that they should > receive their onload events first. In Chromium we took the approach of synchronously sending load events for data uri images and I think you're on a good path here in terms of web compatibility. I can warn you about an edge case that did bite us: global image event handlers. These are a kludge and should be removed entirely but synchronously firing them at inopportune times can be dangerous (e.g., from elements that themselves load images). It may be useful to check out the following patches and possibly include their tests (if only as a safeguard): https://src.chromium.org/viewvc/blink?revision=153029&view=revision https://src.chromium.org/viewvc/blink?view=rev&revision=153969

Created attachment 245904 [details] Patch

I changed the solution to be asynchronous loading as expected for all resources and sub-resources. The solution depends on using the NetworkingContext of the main FrameLoader when loading a data URI resource.

Comment on attachment 245904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245904&action=review Just a few drive-by comments. > Source/WebCore/svg/graphics/SVGImageClients.h:36 > +class SVGImageChromeClient : public EmptyChromeClient { Is this ever subclassed? if no, we should make it final. > Source/WebCore/svg/graphics/SVGImageClients.h:50 > + m_image = 0; nullptr > Source/WebCore/svg/graphics/SVGImageClients.h:63 > +inline SVGImageChromeClient* toSVGImageChromeClient(ChromeClient* client) We use downcast<>() instead of those toXXX() in WebKit nowadays. You would use need to use SPECIALIZE_TYPE_TRAITS_*() to provide the right Traits specialization and then downcast<>() would work for SVGImageChromeClient as well. > Source/WebCore/svg/graphics/SVGImageClients.h:76 > + virtual FrameLoader* dataProtocolLoader() const { return m_dataProtocolLoader; } missing override?

Created attachment 245921 [details] Patch

(In reply to comment #82) > Comment on attachment 245904 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245904&action=review > > Just a few drive-by comments. > > > Source/WebCore/svg/graphics/SVGImageClients.h:36 > > +class SVGImageChromeClient : public EmptyChromeClient { > > Is this ever subclassed? if no, we should make it final. > Done. > > Source/WebCore/svg/graphics/SVGImageClients.h:50 > > + m_image = 0; > > nullptr > Done. > > Source/WebCore/svg/graphics/SVGImageClients.h:63 > > +inline SVGImageChromeClient* toSVGImageChromeClient(ChromeClient* client) > > We use downcast<>() instead of those toXXX() in WebKit nowadays. You would > use need to use SPECIALIZE_TYPE_TRAITS_*() to provide the right Traits > specialization and then downcast<>() would work for SVGImageChromeClient as > well. > toSVGImageChromeClient() is deleted because it is never referenced in the code. > > Source/WebCore/svg/graphics/SVGImageClients.h:76 > > + virtual FrameLoader* dataProtocolLoader() const { return m_dataProtocolLoader; } > > missing override? Done.

Comment on attachment 245921 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245921&action=review I think the approach here is wrong. We should find a way to connect the SVGImage with the loader that does not involve adding the FrameLoader concept to the Image class. Unfortunately, it looks like the entire SVGImage mechanism itself has a similar layering violation, but I don’t think it needs to. I think the main thing would be to move the SVGImage object out of the platform directory, since it’s a much higher level concept. > Source/WebCore/loader/ResourceLoader.cpp:176 > + FrameLoader* loader = m_request.url().protocolIsData() ? dataProtocolFrameLoader() : frameLoader(); Since we rely on this never being null, I suggest making this local variable be a reference rather than a pointer. > Source/WebCore/loader/ResourceLoader.cpp:205 > + FrameLoader* loader = frameLoader(); > + return loader && loader->client().dataProtocolLoader() ? loader->client().dataProtocolLoader() : loader; It’s not good that this calls loader->client().dataProtocolLoader() twice. It should be rewritten to not do that. I suggest changing this to return FrameLoader& and to assert that frameLoader() is non-null. We only call this when we need it to be non-null, so it’s not good to have the null checking and to have a pointer at the call site. > Source/WebCore/loader/cache/CachedImage.cpp:112 > + if (cachedResourceLoader.autoLoadImages() || resourceRequest().url().protocolIsData()) Can we find a way to put this logic into CachedResourceLoader rather than repeating the logic twice? It’s not good that both this function and CachedResourceLoader::shouldDeferImageLoad encode the same rule. > Source/WebCore/platform/graphics/Image.h:114 > + virtual void setDataProtocolLoader(FrameLoader*) { } This is not the right way to design this; it’s a layering violation. The Image class is a lower level graphics abstraction from the platform directory and must not know anything about frame loaders. Even though it’s uglier, at the very least we should use isSVGImage and put this function on the SVG image class only. But we should also look for opportunities to fix this. We do not want our graphics level Image class depend on the design of the frame loader!

I think that long term we need some kind of ImageLoadingContext that the graphics layer uses to abstract what it needs from the higher levels. That’s where NetworkingContext came from; at one time code was working directly with Frame and Page, but then we decided to abstract that for the loader. But a problem here is that SVGImage is not really part of the graphics layer. We need to figure out a factoring where SVGImage can be part of the higher level of SVG, not of the graphics layer, and SVGImage can get the data it needs without the Image class having to have an isSVGImage function in it or having knowledge of loaders. In any case, we should not take any more steps in the direction of building knowledge of the loader into the graphics layer.

Created attachment 245968 [details] Patch

(In reply to comment #85) > Comment on attachment 245921 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245921&action=review > > I think the approach here is wrong. We should find a way to connect the > SVGImage with the loader that does not involve adding the FrameLoader > concept to the Image class. Unfortunately, it looks like the entire SVGImage > mechanism itself has a similar layering violation, but I don’t think it > needs to. I think the main thing would be to move the SVGImage object out of > the platform directory, since it’s a much higher level concept. > > > Source/WebCore/loader/ResourceLoader.cpp:176 > > + FrameLoader* loader = m_request.url().protocolIsData() ? dataProtocolFrameLoader() : frameLoader(); > > Since we rely on this never being null, I suggest making this local variable > be a reference rather than a pointer. > Done. And I added an assertion that frameLoader() is not null. > > Source/WebCore/loader/ResourceLoader.cpp:205 > > + FrameLoader* loader = frameLoader(); > > + return loader && loader->client().dataProtocolLoader() ? loader->client().dataProtocolLoader() : loader; > > It’s not good that this calls loader->client().dataProtocolLoader() twice. > It should be rewritten to not do that. > > I suggest changing this to return FrameLoader& and to assert that > frameLoader() is non-null. We only call this when we need it to be non-null, > so it’s not good to have the null checking and to have a pointer at the call > site. > Done. > > Source/WebCore/loader/cache/CachedImage.cpp:112 > > + if (cachedResourceLoader.autoLoadImages() || resourceRequest().url().protocolIsData()) > > Can we find a way to put this logic into CachedResourceLoader rather than > repeating the logic twice? It’s not good that both this function and > CachedResourceLoader::shouldDeferImageLoad encode the same rule. > Done. A new function called shouldPerformImageLoad() is added to CachedResourceLoader. If the name is not clear enough, please let me know. > > Source/WebCore/platform/graphics/Image.h:114 > > + virtual void setDataProtocolLoader(FrameLoader*) { } > > This is not the right way to design this; it’s a layering violation. The > Image class is a lower level graphics abstraction from the platform > directory and must not know anything about frame loaders. Even though it’s > uglier, at the very least we should use isSVGImage and put this function on > the SVG image class only. But we should also look for opportunities to fix > this. We do not want our graphics level Image class depend on the design of > the frame loader! Done. setDataProtocolLoader() is now a specific function to the SVGImage. It is called from CachedImage::finishedLoading() only when m_image->isSVGImage(). The same condition is repeated in couple places in CachedImage.cpp.

Comment on attachment 245968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245968&action=review > Source/WebCore/svg/graphics/SVGImage.h:109 > + FrameLoader* m_dataProtocolLoader; If we initialized this to null here we would not need to in the constructor: FrameLoader* m_dataProtocolLoader { nullptr };

<rdar://problem/19717682>

Created attachment 246039 [details] Patch

Comment on attachment 246039 [details] Patch Clearing flags on attachment: 246039 Committed r179626: <http://trac.webkit.org/changeset/179626>

All reviewed patches have been landed. Closing bug.

Holy knights of columbus! It's been fixed!!!

*** Bug 127441 has been marked as a duplicate of this bug. ***