Bug 40177 – CachedResourceHandle isn't held long enough to guaranteed CachedScript ownership

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Tony Gentilcore]

CachedResourceHandle isn't held long enough to guaranteed CachedScript ownership

[Tony Gentilcore]

Created attachment 57918 [details]
Patch

[Tony Gentilcore]

abarth - you brought up this issue in https://bugs.webkit.org/show_bug.cgi?id=40089

[Tony Gentilcore]

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.

[Darin Adler]

(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.

[Tony Gentilcore]

(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 :)

[Alexey Proskuryakov]

+        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.

[Tony Gentilcore]

(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.

[Eric Seidel (no email)]

Could this be related to the invalid CachedResource* crashes we were seeing for a while with the HTML5 parser?

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Sorry, I renamed *Tokenizer out from under you!

[Tony Gentilcore]

Created attachment 58660 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 58660 [details]
Patch

Clearing flags on attachment: 58660

Committed r61306: <http://trac.webkit.org/changeset/61306>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.