If we have an HTML document with an image, and we change the image src to an url that is blocked by content-security-policy, the document itself will never be released. Below is the test case: <html> <head> <meta http-equiv="Content-Security-Policy" content="img-src 'self'"> <script> function load() { setTimeout(function() { var image = document.getElementById('test'); image.src = 'http://www.mysample.com/image.png'; location.reload(); }, 300); } </script> </head> <body onload='setTimeout(load(), 100)'> <img id='test' src="./test.png"></img> </body> </html> Loading this into Safari and run 'heap WebProcess | grep HTMLDocument", you will see the number of living document keeps growing.
<rdar://problem/13128478>
Created attachment 185903 [details] Don't cancel error event if the newImage is blocked due to content security policy.
Comment on attachment 185903 [details] Don't cancel error event if the newImage is blocked due to content security policy. View in context: https://bugs.webkit.org/attachment.cgi?id=185903&action=review Great catch. Another potential way to fix this bug would be do dispatch the error event immediately in this case, not through errorEventSender().dispatchEventSoon(). That would be observable through JavaScript, and I don't know which behavior is right. Can you find it out from the spec and from other browsers' behavior? Note that we currently dispatch the event synchronously for an empty URL. I do not fully understand why canceling the error event causes a world leak. Is there something else going wrong in lower level code? > Source/WebCore/ChangeLog:12 > + No new tests, manually verified by using heap tool in OS X Safari. It is difficult to test for abandoned memory, but can we test for the event being fired?
(In reply to comment #3) > (From update of attachment 185903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185903&action=review > > Great catch. > > Another potential way to fix this bug would be do dispatch the error event immediately in this case, not through errorEventSender().dispatchEventSoon(). That would be observable through JavaScript, and I don't know which behavior is right. Can you find it out from the spec and from other browsers' behavior? Looks like the spec does expect to fire the error event asynchronously, from http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content-1.html#the-img-element (section 12): "If selected source is null, then set the element to the broken state, queue a task to fire a simple event named error at the img element" > > Note that we currently dispatch the event synchronously for an empty URL. > This sounds like behaves differently from the spec. > I do not fully understand why canceling the error event causes a world leak. Is there something else going wrong in lower level code? In ImageLoader::updateFromElement, when the image url is blocked by cross-site-voilation or content-security-policy, newImage is null and we schedule error event in ImageLoader.cpp:208: m_hasPendingErrorEvent = true; errorEventSender().dispatchEventSoon(this); Later, we cancel the error event in the same call stack since m_hasPendingErrorEvent is true, in ImageLoader.cpp:226: if (m_hasPendingErrorEvent) errorEventSender().cancelEvent(this); At the end of ImageLoader::updateFromElement, we ref the source Element in updateHasPendingEvent() since m_hasPendingErrorEvent is still true, at ImageLoader.cpp:256: updatedHasPendingEvent(); At this point, the error event is cancelled so errorEventSender won't call dispatchPendingErrorEvents for this ImageLoader any more. However, the source Element is still ref-ed and we will leak. > > > Source/WebCore/ChangeLog:12 > > + No new tests, manually verified by using heap tool in OS X Safari. > > It is difficult to test for abandoned memory, but can we test for the event being fired? Good idea! I will make a test for that.
> At the end of ImageLoader::updateFromElement, we ref the source Element in updateHasPendingEvent() since m_hasPendingErrorEvent is still true, at ImageLoader.cpp:256: This makes me wonder if we should set m_hasPendingErrorEvent to false when calling errorEventSender().cancelEvent(this).
(In reply to comment #5) > > At the end of ImageLoader::updateFromElement, we ref the source Element in updateHasPendingEvent() since m_hasPendingErrorEvent is still true, at ImageLoader.cpp:256: > > This makes me wonder if we should set m_hasPendingErrorEvent to false when calling errorEventSender().cancelEvent(this). We could... but then we won't fire the error event in this case.
I guess what I'm saying is that we have two bugs here. 1. A leak of the whole document. The best fix for this could be to accurately track m_hasPendingErrorEvent, always unsetting it after cancelEvent(). 2. Error event not firing. The best fix for this is probably the one that's posted. Not necessarily implying that these should be fixed in separate patches, but that's how I think about it.
(In reply to comment #7) > I guess what I'm saying is that we have two bugs here. > > 1. A leak of the whole document. The best fix for this could be to accurately track m_hasPendingErrorEvent, always unsetting it after cancelEvent(). > > 2. Error event not firing. The best fix for this is probably the one that's posted. > > Not necessarily implying that these should be fixed in separate patches, but that's how I think about it. I see, thanks! I will make a new patch to fix both. Anyway, the test will only be able to test if error event is fired.
Created attachment 186133 [details] add an test to verify error event is fired.
Comment on attachment 186133 [details] add an test to verify error event is fired. View in context: https://bugs.webkit.org/attachment.cgi?id=186133&action=review This looks pretty good. I had a number of comments, and if you choose to address certain of those, it would be beneficial to run the new patch by EWS, so marking r- for now. > Source/WebCore/loader/ImageLoader.cpp:223 > beforeLoadEventSender().cancelEvent(this); > if (m_hasPendingLoadEvent) > loadEventSender().cancelEvent(this); These functions should also unset their respective data members (m_hasPendingBeforeLoadEvent, m_hasPendingLoadEvent). You can compare this function to what setImageWithoutConsideringPendingLoadEvent() does. > Source/WebCore/loader/ImageLoader.cpp:224 > + // Don't cancel the error event if it is just scheduled due to invalid newImage. I think that "due to invalid newImage" is a bit misleading. The image is null, and the reason for posting the event was that the load was not permitted, not that the image was invalid. It is already an issue for existing code, which has a very long comment to explain why a null check for newImage translates into posting an error event. It would be better to express this in a more direct way. In the meanwhile, I'd say something more along the lines of // Cancel error events that belong to the previous load, which is now cancelled by changing the src attribute. // If newImage is null, we assume that any error event has been just posted by this function. // FIXME: If both previous load and this one got blocked with an error, we can receive one error event instead of two. > LayoutTests/fast/images/image-error-event-not-firing-expected.txt:11 > +PASS successfullyParsed is true > + > +TEST COMPLETE > +PASS error event fired. The way to avoid broken output order is to set window.jsTestIsAsync, and to then call finishJSTest() instead of notifyDone().
Created attachment 186180 [details] Patch, addressing review comments.
Comment on attachment 186180 [details] Patch, addressing review comments. View in context: https://bugs.webkit.org/attachment.cgi?id=186180&action=review > LayoutTests/fast/images/image-error-event-not-firing.html:25 > + }, 200); Would a zero timeout work?
(In reply to comment #12) > (From update of attachment 186180 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186180&action=review > > > LayoutTests/fast/images/image-error-event-not-firing.html:25 > > + }, 200); > > Would a zero timeout work? My concern is using zero timeout could cause flaky results since onerror is fired asynchronously and errorEventSender's timer might fire after setTimeout DOMTimer. Setting to 200 gives us a safe buffer for that.
Comment on attachment 186180 [details] Patch, addressing review comments. Clearing flags on attachment: 186180 Committed r141667: <http://trac.webkit.org/changeset/141667>
All reviewed patches have been landed. Closing bug.