[Webkit-unassigned] [Bug 108545] Document is never released if an image's src attribute is changed to a url blocked by content-security-policy.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=108545


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #186133|review?                     |review-
               Flag|                            |




--- Comment #10 from Alexey Proskuryakov <ap at webkit.org>  2013-02-01 14:37:03 PST ---
(From update of attachment 186133)
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().

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.