REGRESSION: crash in ScriptElement::notifyFinished

[Darin Fisher (:fishd, Google)]

REGRESSION: crash in ScriptElement::notifyFinished

I observed this crash on http://www.kino.de/kinosuche.php4
(you might have to load that site a couple times to repro the crash)

I found that some code changed recently that probably explains the crash.

Compare:

-void HTMLScriptElement::notifyFinished(CachedResource* o)
-{
-    CachedScript* cs = static_cast<CachedScript*>(o);
-
-    ASSERT(cs == m_cachedScript);
-
-    // Evaluating the script could lead to a garbage collection which
-    // can delete the script element so we need to protect it.
-    RefPtr<HTMLScriptElement> protect(this);
-    
-    if (cs->errorOccurred())
-        dispatchHTMLEvent(errorEvent, true, false);
-    else {
-        evaluateScript(cs->url(), cs->script());
-        dispatchHTMLEvent(loadEvent, false, false);
-    }
-
-    // script evaluation may have dereffed it already
-    if (m_cachedScript) {
-        m_cachedScript->removeClient(this);
-        m_cachedScript = 0;
-    }
-}

To:

void ScriptElementData::notifyFinished(CachedResource* o)
{
    CachedScript* cs = static_cast<CachedScript*>(o);
    ASSERT(cs == m_cachedScript);

    if (cs->errorOccurred())
        m_scriptElement->dispatchErrorEvent();
    else {
        RefPtr<Element> protector(m_element);
        evaluateScript(cs->url(), cs->script());
        m_scriptElement->dispatchLoadEvent();
    }

    stopLoadRequest();
}

The crash I get is inside stopLoadRequest() with |this| having been trashed.  m_element owns |this|.

Here's the interesting portion of the crash stack:

 	chrome.dll!WTF::HashTable<WebCore::RenderObject *,std::pair<WebCore::RenderObject *,WebCore::RenderBlock::FloatingObject 
*>,WTF::PairFirstExtractor<std::pair<WebCore::RenderObject *,WebCore::RenderBlock::FloatingObject *> >,WTF::PtrHash<WebCore::RenderObject 
*>,WTF::PairHashTraits<WTF::HashTraits<WebCore::RenderObject *>,WTF::HashTraits<WebCore::RenderBlock::FloatingObject *> 
>,WTF::HashTraits<WebCore::RenderObject *> >::find<WebCore::RenderObject *,WTF::IdentityHashTranslator<WebCore::RenderObject 
*,std::pair<WebCore::RenderObject *,WebCore::RenderBlock::FloatingObject *>,WTF::PtrHash<WebCore::RenderObject *> > >(WebCore::RenderObject * const & 
key=0x0232775c)  Line 751	C++
 	chrome.dll!WebCore::CachedResource::removeClient(WebCore::CachedResourceClient * c=0x0232775c)  Line 131 + 0x12 bytes	C++
>	chrome.dll!WebCore::ScriptElementData::notifyFinished(WebCore::CachedResource * o=0x02330e58)  Line 191 + 0xd bytes	C++
 	chrome.dll!WebCore::CachedScript::checkNotify()  Line 95 + 0xa bytes	C++
 	chrome.dll!WebCore::CachedScript::data(WTF::PassRefPtr<WebCore::SharedBuffer> data={...}, bool allDataReceived=true)  Line 85 + 0xe bytes	
C++
 	chrome.dll!WebCore::Loader::Host::didFinishLoading(WebCore::SubresourceLoader * loader=0x034297f8)  Line 278	C++
 	chrome.dll!WebCore::SubresourceLoader::didFinishLoading()  Line 195	C++
 	chrome.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x01f31cf8)  Line 399	C++

Originally identified here:
http://code.google.com/p/chromium/issues/detail?id=3056

In the old code, the protector was also ensuring that |this| remained valid until after stopLoadRequest() is called.  I think this is an apparent goof in the recent code change.

[Alexey Proskuryakov]

(In reply to comment #0)
> I found that some code changed recently that probably explains the crash.

<http://trac.webkit.org/changeset/35744>.

[Darin Fisher (:fishd, Google)]

This patch, which essentially just rolls back part of the original change, seems to resolve the bug.  I'll work on a layout test, but it may be challenging since it is dependent on GC running at the right time.

Index: ScriptElement.cpp
===================================================================
--- ScriptElement.cpp   (revision 2802)
+++ ScriptElement.cpp   (working copy)
@@ -180,10 +180,13 @@
     CachedScript* cs = static_cast<CachedScript*>(o);
     ASSERT(cs == m_cachedScript);
+    // Evaluating the script could lead to a garbage collection which can
+    // delete the script element so we need to protect it.
+    RefPtr<Element> protector(m_element);
+
     if (cs->errorOccurred())
         m_scriptElement->dispatchErrorEvent();
     else {
-        RefPtr<Element> protector(m_element);
         evaluateScript(cs->url(), cs->script());
         m_scriptElement->dispatchLoadEvent();
     }

[Eric Seidel (no email)]

(In reply to comment #2)
> This patch, which essentially just rolls back part of the original change,
> seems to resolve the bug.  I'll work on a layout test, but it may be
> challenging since it is dependent on GC running at the right time.
> 
> Index: ScriptElement.cpp
> ===================================================================
> --- ScriptElement.cpp   (revision 2802)
> +++ ScriptElement.cpp   (working copy)
> @@ -180,10 +180,13 @@
>      CachedScript* cs = static_cast<CachedScript*>(o);
>      ASSERT(cs == m_cachedScript);
> +    // Evaluating the script could lead to a garbage collection which can
> +    // delete the script element so we need to protect it.
> +    RefPtr<Element> protector(m_element);
> +
>      if (cs->errorOccurred())
>          m_scriptElement->dispatchErrorEvent();
>      else {
> -        RefPtr<Element> protector(m_element);
>          evaluateScript(cs->url(), cs->script());
>          m_scriptElement->dispatchLoadEvent();
>      }
> 

The change looks good.  We just need a changelog, and ideally a layout test.

gc() or GCController.collect() should be able to kick off GC when you need.

[Darin Fisher (:fishd, Google)]

Created attachment 24076 [details]
v1 patch

Here's the patch sans layout test.  Try as I might, I could not get my Mac build of WebKit to crash upon reaching ScriptElementData::stopLoadRequest despite having a junk |this| pointer.

[Darin Fisher (:fishd, Google)]

Created attachment 24077 [details]
v2 patch - same as before, but includes bug title in ChangeLog

I tried using GuardMalloc to help create a test, but it seems like there is something else besides this issue that is causing DRT to crash under GuardMalloc.  For example, fast/dom/script-element-gc.html crashes with or without my fix under GuardMalloc.  I don't know how to get a call stack when GuardMalloc detects an error, so I don't know why DRT is crashing under GuardMalloc.

[Eric Seidel (no email)]

Comment on attachment 24077 [details]
v2 patch - same as before, but includes bug title in ChangeLog

Looks great.  Thanks for fixing this.  It would be better w/ a layout test, but from discussions in the channel it sounds like you were having trouble getting one to work on your Mac.

I think you have commit-bit these days, so you should be able to land this yourself.

[Eric Seidel (no email)]

(In reply to comment #5)
> Created an attachment (id=24077) [edit]
> v2 patch - same as before, but includes bug title in ChangeLog
> 
> I tried using GuardMalloc to help create a test, but it seems like there is
> something else besides this issue that is causing DRT to crash under
> GuardMalloc.  For example, fast/dom/script-element-gc.html crashes with or
> without my fix under GuardMalloc.  I don't know how to get a call stack when
> GuardMalloc detects an error, so I don't know why DRT is crashing under
> GuardMalloc.
> 

We need to file any --guard crashes for run-webkit-tests as bugs.   Or rather, when you see any, don't hesitate to file them.  We really should have a buildbot which runs run-webkit-tests --guard once a week or something.

[Darin Fisher (:fishd, Google)]

I filed bug 21380 about how "run-webkit-tests -g" just crashes for me on seemingly every test.

[Nikolas Zimmermann]

Oops, good catch! My fault while refactoring the code... Sorry for any headaches.