[webkit-reviews] review denied: [Bug 108545] Document is never released if an image's src attribute is changed to a url blocked by content-security-policy. : [Attachment 186133] add an test to verify error event is fired.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Yongjun Zhang
<yongjun_zhang at apple.com>'s request for review:
Bug 108545: Document is never released if an image's src attribute is changed
to a url blocked by content-security-policy.
https://bugs.webkit.org/show_bug.cgi?id=108545

Attachment 186133: add an test to verify error event is fired.
https://bugs.webkit.org/attachment.cgi?id=186133&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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().