CachedResourceHandle isn't held long enough to guaranteed CachedScript ownership
Created attachment 57918 [details] Patch
abarth - you brought up this issue in https://bugs.webkit.org/show_bug.cgi?id=40089
By the way, I had a related question. It looks to me like CachedResourceHandle could just go away in favor of RefPtr. Am I missing something? If not, I could look into cleaning that up.
(In reply to comment #3) > It looks to me like CachedResourceHandle could just go away in favor of RefPtr. Am I missing something? If not, I could look into cleaning that up. You are missing something. Look at m_handlesToRevalidate in CachedResource to see what this does beyond what RefPtr does.
(In reply to comment #4) > (In reply to comment #3) > > It looks to me like CachedResourceHandle could just go away in favor of RefPtr. Am I missing something? If not, I could look into cleaning that up. > > You are missing something. Look at m_handlesToRevalidate in CachedResource to see what this does beyond what RefPtr does. Oh, I see now. Thanks for pointing that out :)
+ No new tests because no new functionality. Do you know what is holding the other reference(s)? If this patch actually fixes a bug, then landing a regression test would be way more important than even fixing it - this area isn't well covered by tests.
(In reply to comment #6) > + No new tests because no new functionality. > > Do you know what is holding the other reference(s)? If this patch actually fixes a bug, then landing a regression test would be way more important than even fixing it - this area isn't well covered by tests. I don't believe there is actually a bug here. It just seems like booby trap that could be triggered by an unrelated change. When I trace through, I always have at least 1 other reference to the CachedScript. It can be from the DocLoader::m_documentResources and/or from the ScriptElement::m_cachedScript.
Could this be related to the invalid CachedResource* crashes we were seeing for a while with the HTML5 parser?
Comment on attachment 57918 [details] Patch Rejecting patch 57918 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1 Last 500 characters of output: WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. can't find file to patch at input line 5 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |Index: WebCore/html/HTMLTokenizer.cpp |index 629e7f4187448db7f0516add263995dc5e9e2636..74063ddc5c91bc25c2208bdf52e33d5e557f4919 100644 |--- WebCore/html/HTMLTokenizer.cpp |+++ WebCore/html/HTMLTokenizer.cpp -------------------------- No file to patch. Skipping patch. 2 out of 2 hunks ignored Full output: http://webkit-commit-queue.appspot.com/results/3309015
Sorry, I renamed *Tokenizer out from under you!
Created attachment 58660 [details] Patch
Comment on attachment 58660 [details] Patch Clearing flags on attachment: 58660 Committed r61306: <http://trac.webkit.org/changeset/61306>
All reviewed patches have been landed. Closing bug.