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+

Gavin Peters
Reported 2011-10-05 12:26:33 PDT
save resource zeroing stacks in ScriptElement for debugging
Attachments
Patch (3.13 KB, patch)
2011-10-05 12:29 PDT, Gavin Peters
no flags
Patch (3.13 KB, patch)
2011-10-05 12:35 PDT, Gavin Peters
no flags
Patch (3.27 KB, patch)
2011-10-05 12:55 PDT, Gavin Peters
no flags
Patch (3.25 KB, patch)
2011-10-06 06:50 PDT, Gavin Peters
japhet: review+
japhet: commit-queue+
Gavin Peters
Comment 1 2011-10-05 12:29:21 PDT
Gavin Peters
Comment 2 2011-10-05 12:30:33 PDT
add more data for crashes in ScriptElement::notifyFinished double calls.
Adam Barth
Comment 3 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
Gavin Peters
Comment 4 2011-10-05 12:35:18 PDT
Gavin Peters
Comment 5 2011-10-05 12:36:06 PDT
Comment on attachment 109838 [details] Patch Fixed indent.
Nate Chapin
Comment 6 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?
Gavin Peters
Comment 7 2011-10-05 12:55:04 PDT
Gavin Peters
Comment 8 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.
Gavin Peters
Comment 9 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?
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2011-10-05 15:02:46 PDT
All reviewed patches have been landed. Closing bug.
Adam Roben (:aroben)
Comment 12 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.
Gavin Peters
Comment 13 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.
Gavin Peters
Comment 14 2011-10-06 06:50:52 PDT
Gavin Peters
Comment 15 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?
Nate Chapin
Comment 16 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.
Gavin Peters
Comment 17 2011-10-06 09:40:15 PDT
Note You need to log in before you can comment on or make changes to this bug.