| Summary: | REGRESSION (r85375): Load event is sometimes lost when multiple image elements use the same URL | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||
| Component: | Images | Assignee: | Alexey Proskuryakov <ap> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ademar, ggaren | ||||||
| Priority: | P1 | Keywords: | InRadar, Regression | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Alexey Proskuryakov
2011-05-28 14:22:18 PDT
Created attachment 95275 [details]
proposed fix
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? 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.
> 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. Created attachment 95277 [details]
proposed fix
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". Committed <http://trac.webkit.org/changeset/87628> with an additional simple fix for massive test failure. 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()); Changed to plain ASSERT in <http://trac.webkit.org/changeset/87633> (no effect on release or debug builds). 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> |