Bug 108545

Summary:

Document is never released if an image's src attribute is changed to a url blocked by content-security-policy.

Product:

WebKit

Reporter:

Yongjun Zhang <yongjun_zhang>

Component:

Images

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, ddkilzer, japhet, webkit.review.bot, yongjun_zhang

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Don't cancel error event if the newImage is blocked due to content security policy.	none
add an test to verify error event is fired.	ap: review-
Patch, addressing review comments.	none

Yongjun Zhang

Reported 2013-01-31 16:03:44 PST

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.

Attachments
Don't cancel error event if the newImage is blocked due to content security policy. (1.82 KB, patch) 2013-01-31 17:21 PST, Yongjun Zhang	no flags	Details Formatted Diff Diff
add an test to verify error event is fired. (4.89 KB, patch) 2013-02-01 14:05 PST, Yongjun Zhang	ap: review-	Details Formatted Diff Diff
Patch, addressing review comments. (5.39 KB, patch) 2013-02-01 16:05 PST, Yongjun Zhang	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yongjun Zhang

Comment 1 2013-01-31 16:04:36 PST

<rdar://problem/13128478>

Yongjun Zhang

Comment 2 2013-01-31 17:21:51 PST

Created attachment 185903 [details] Don't cancel error event if the newImage is blocked due to content security policy.

Alexey Proskuryakov

Comment 3 2013-02-01 10:24:29 PST

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?

Yongjun Zhang

Comment 4 2013-02-01 11:48:02 PST

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

Alexey Proskuryakov

Comment 5 2013-02-01 12:03:36 PST

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

Yongjun Zhang

Comment 6 2013-02-01 12:06:01 PST

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

Alexey Proskuryakov

Comment 7 2013-02-01 12:51:07 PST

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.

Yongjun Zhang

Comment 8 2013-02-01 13:49:41 PST

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

Yongjun Zhang

Comment 9 2013-02-01 14:05:29 PST

Created attachment 186133 [details] add an test to verify error event is fired.

Alexey Proskuryakov

Comment 10 2013-02-01 14:37:03 PST

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

Yongjun Zhang

Comment 11 2013-02-01 16:05:28 PST

Created attachment 186180 [details] Patch, addressing review comments.

Alexey Proskuryakov

Comment 12 2013-02-01 16:11:20 PST

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?

Yongjun Zhang

Comment 13 2013-02-01 16:28:47 PST

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

WebKit Review Bot

Comment 14 2013-02-01 16:59:59 PST

Comment on attachment 186180 [details] Patch, addressing review comments. Clearing flags on attachment: 186180 Committed r141667: <http://trac.webkit.org/changeset/141667>

WebKit Review Bot

Comment 15 2013-02-01 17:00:03 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.