Bug 69453

Summary: save resource zeroing stacks in ScriptElement for debugging
Product: WebKit Reporter: Gavin Peters <gavinp>
Component: New BugsAssignee: Gavin Peters <gavinp>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ap, aroben, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 69472    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch japhet: review+, japhet: commit-queue+

Description Gavin Peters 2011-10-05 12:26:33 PDT
save resource zeroing stacks in ScriptElement for debugging
Comment 1 Gavin Peters 2011-10-05 12:29:21 PDT
Created attachment 109837 [details]
Patch
Comment 2 Gavin Peters 2011-10-05 12:30:33 PDT
add more data for crashes in ScriptElement::notifyFinished double calls.
Comment 3 Adam Barth 2011-10-05 12:33:00 PDT
Comment on attachment 109837 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109837&action=review

> Source/WebCore/dom/ScriptElement.h:120
> +      MaxBacktraceSize = 32

bad indent
Comment 4 Gavin Peters 2011-10-05 12:35:18 PDT
Created attachment 109838 [details]
Patch
Comment 5 Gavin Peters 2011-10-05 12:36:06 PDT
Comment on attachment 109838 [details]
Patch

Fixed indent.
Comment 6 Nate Chapin 2011-10-05 12:50:37 PDT
Comment on attachment 109838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109838&action=review

> Source/WebCore/ChangeLog:8
> +        two notifyFinished() events in a ScriptElement, which can crash chrome (see chrome bug
> +        75604).  This patch will save a stack in the ScriptElement when this happens, so that if

Please provide a link to the chromium bug.

> Source/WebCore/dom/ScriptElement.h:122
> +
> +    // We grab a backtrace when we zero m_cachedScript, so that at later crashes
> +    // we'll have a debuggable stack.
> +    enum {
> +        MaxBacktraceSize = 32
> +    };
> +    int m_backtraceSize;

Does this value ever change?  Can we just make it a static const in ScriptElement.cpp?
Comment 7 Gavin Peters 2011-10-05 12:55:04 PDT
Created attachment 109842 [details]
Patch
Comment 8 Gavin Peters 2011-10-05 12:57:37 PDT
Comment on attachment 109838 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109838&action=review

>> Source/WebCore/ChangeLog:8
>> +        75604).  This patch will save a stack in the ScriptElement when this happens, so that if
> 
> Please provide a link to the chromium bug.

Done.

>> Source/WebCore/dom/ScriptElement.h:122
>> +    int m_backtraceSize;
> 
> Does this value ever change?  Can we just make it a static const in ScriptElement.cpp?

It does change, it contains the actual size of the stack in m_backtrace, it could be anything from 0 to 32.
Comment 9 Gavin Peters 2011-10-05 12:58:07 PDT
Comment on attachment 109842 [details]
Patch

Now with links to the chromium bug database.  Boy, isn't the chromium bug database really unbearably slow?
Comment 10 WebKit Review Bot 2011-10-05 15:02:42 PDT
Comment on attachment 109842 [details]
Patch

Clearing flags on attachment: 109842

Committed r96756: <http://trac.webkit.org/changeset/96756>
Comment 11 WebKit Review Bot 2011-10-05 15:02:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Adam Roben (:aroben) 2011-10-05 15:35:56 PDT
This broke many builds due to WTFGetBacktrace not being exported from JavaScriptCore. Rolling it out in bug 69472.
Comment 13 Gavin Peters 2011-10-06 06:38:13 PDT
I just added a new patch to bug 69018 which exports these functions, after that this patch should be good to go, possibly after rerunning ews.
Comment 14 Gavin Peters 2011-10-06 06:50:52 PDT
Created attachment 109957 [details]
Patch
Comment 15 Gavin Peters 2011-10-06 06:54:01 PDT
Comment on attachment 109957 [details]
Patch

This patch is unchanged from the version that got a rollout; however I've since updated the exports from JavaScriptCore so it should work now.  Let's wait on the windows ews bot in particular, then land it?
Comment 16 Nate Chapin 2011-10-06 09:05:00 PDT
Comment on attachment 109957 [details]
Patch

Looks like it's passed the bots this time, so r+ again.
Comment 17 Gavin Peters 2011-10-06 09:40:15 PDT
Committed r96819: <http://trac.webkit.org/changeset/96819>