Bug 63300

Summary: Stop listening for CachedScript events as soon as notifyFinished() is called
Product: WebKit Reporter: James Simonsen <simonjam>
Component: Page LoadingAssignee: James Simonsen <simonjam>
Status: NEW    
Severity: Normal CC: abarth, ap, mitz, tonyg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ap: review-

James Simonsen
Reported 2011-06-23 16:07:54 PDT
Stop listening for CachedScript events as soon as notifyFinished() is called
Attachments
Patch (1.54 KB, patch)
2011-06-23 16:09 PDT, James Simonsen
ap: review-
James Simonsen
Comment 1 2011-06-23 16:09:39 PDT
James Simonsen
Comment 2 2011-06-23 16:10:29 PDT
Adam Barth
Comment 3 2011-06-23 21:13:37 PDT
Comment on attachment 98426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98426&action=review It's too bad we don't have a good repro for this issue, but this patch looks fine. > Source/WebCore/dom/ScriptElement.cpp:316 > void ScriptElement::notifyFinished(CachedResource* o) > { > ASSERT(!m_willBeParserExecuted); > ASSERT_UNUSED(o, o == m_cachedScript); Can we rename "o" to something more readable, like "resource" ?
Alexey Proskuryakov
Comment 4 2011-06-24 00:06:15 PDT
Comment on attachment 98426 [details] Patch This needs a good explanation of why it's OK to revert r44591 now.
Alexey Proskuryakov
Comment 5 2011-06-24 00:08:26 PDT
I do not know anything special about that patch - I just did svn blame, and immediately got pointed to it.
Tony Gentilcore
Comment 6 2011-06-24 04:09:21 PDT
> This needs a good explanation of why it's OK to revert r44591 now. Yeah, I don't think this fix is quite right. Once all handles are released, the CachedResource's underlying buffer may be purged. In the HTMLScriptRunner we created http://trac.webkit.org/browser/trunk/Source/WebCore/dom/PendingScript.h to get around this by keeping a CachedResourceHandle alive that just ignores notifyFinished() calls (see comment at top of file). Perhaps it would be useful here too?
Note You need to log in before you can comment on or make changes to this bug.