Bug 61692 – REGRESSION (r85375): Load event is sometimes lost when multiple image elements use the same URL

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Alexey Proskuryakov]

Patch forthcoming.

<rdar://problem/9488628>

[Alexey Proskuryakov]

Created attachment 95275 [details]
proposed fix

[Darin Adler]

Comment on attachment 95275 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95275&action=review

> Source/WebCore/html/HTMLImageElement.h:76
> +    bool hasPendingActivity() const;

I’m not sure it’s useful to have this separate function any more if it’s really the same as !haveFiredLoadEvent. Is this a virtual function? Is there a reason callers need to ask this more abstract question instead of asking haveFiredLoadEvent? Is there a reason this is not inlined?

[Geoffrey Garen]

Comment on attachment 95275 [details]
proposed fix

This patch reintroduces a memory leak in the case of a canceled load. Alexey and I are working on a fix.

[Alexey Proskuryakov]

> Is there a reason callers need to ask this more abstract question instead of asking haveFiredLoadEvent?

Yes, I think that callers want the answer to the general question related to garbage collection, which matches the ActiveDOMObject one.

> Is there a reason this is not inlined?

Hmm, maybe it should be now.

[Alexey Proskuryakov]

Created attachment 95277 [details]
proposed fix

[Geoffrey Garen]

Comment on attachment 95277 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95277&action=review

r=me

> Source/WebCore/ChangeLog:44
> +        (WebCore::ImageEventSender::dispatchPendingEvents): Clear the current loader fro dispatching

Typo: "fro".

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/87628> with an additional simple fix for massive test failure.

[Simon Fraser (smfr)]

Comment on attachment 95277 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=95277&action=review

> Source/WebCore/loader/ImageLoader.cpp:228
> +    ASSERT_UNUSED(m_image, resource == m_image.get());

Shouldn't this be
ASSERT_UNUSED(resource, resource == m_image.get());

[Alexey Proskuryakov]

Changed to plain ASSERT in <http://trac.webkit.org/changeset/87633> (no effect on release or debug builds).

[Ademar Reis]

Revision r87628 cherry-picked into qtwebkit-2.2 with commit db21b6f <http://gitorious.org/webkit/qtwebkit/commit/db21b6f>
Revision r87633 cherry-picked into qtwebkit-2.2 with commit 855622e <http://gitorious.org/webkit/qtwebkit/commit/855622e>