Bug 89410 - JSC needs to record line numbers for exception stack traces
Summary: JSC needs to record line numbers for exception stack traces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-06-18 19:08 PDT by Mark Lam
Modified: 2012-06-20 18:58 PDT (History)
1 user (show)

See Also:


Attachments
Fix (10.04 KB, patch)
2012-06-18 19:44 PDT, Mark Lam
ggaren: review-
Details | Formatted Diff | Diff
Revised with Geoff's feedback applied. (9.83 KB, patch)
2012-06-18 21:39 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2012-06-18 19:08:25 PDT
The current implementation of JSC only records line number information if the web inspector is attached.  This needs to be changed so that that exception traces will always have the proper line number info regardless of whether the inspector is attached or not.
Comment 1 Mark Lam 2012-06-18 19:09:10 PDT
Reference: <rdar://problem/11561479>.
Comment 2 Mark Lam 2012-06-18 19:44:37 PDT
Created attachment 148229 [details]
Fix
Comment 3 WebKit Review Bot 2012-06-18 19:46:33 PDT
Comment on attachment 148229 [details]
Fix

Rejecting attachment 148229 [details] from commit-queue.

mark.lam@apple.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 4 Geoffrey Garen 2012-06-18 20:20:27 PDT
Comment on attachment 148229 [details]
Fix

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

Approach looks good, but this patch could use some tweaks before landing.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1603
> +    , m_lineInfo()

"m_lineInfo(other.m_lineInfo)" would be better here, to avoid zero-initializing m_lineInfo just to overwrite it later.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1653
> +    , m_lineInfo()

WebKit style is to omit implicit initializers like this one.

> Source/JavaScriptCore/bytecode/CodeBlock.h:726
> +            return m_lineInfo.size() || (m_rareData

m_lineInfo.size() is always > 0 now, right? If so, I think you should just return true here, and we can remove this optimization in another patch.
Comment 5 Mark Lam 2012-06-18 21:39:29 PDT
Created attachment 148241 [details]
Revised with Geoff's feedback applied.
Comment 6 Geoffrey Garen 2012-06-18 21:49:10 PDT
Comment on attachment 148241 [details]
Revised with Geoff's feedback applied.

r=me
Comment 7 WebKit Review Bot 2012-06-18 23:07:44 PDT
Comment on attachment 148241 [details]
Revised with Geoff's feedback applied.

Clearing flags on attachment: 148241

Committed r120676: <http://trac.webkit.org/changeset/120676>
Comment 8 WebKit Review Bot 2012-06-18 23:07:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2012-06-20 18:51:44 PDT
Comment on attachment 148241 [details]
Revised with Geoff's feedback applied.

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

> Source/JavaScriptCore/bytecode/CodeBlock.h:727
> +            return true || (m_rareData
> +                && (m_rareData->m_expressionInfo.size() || m_rareData->m_exceptionHandlers.size()));

What’s the true || doing here?!
Comment 10 Mark Lam 2012-06-20 18:58:02 PDT
(In reply to comment #9)
> (From update of attachment 148241 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148241&action=review
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.h:727
> > +            return true || (m_rareData
> > +                && (m_rareData->m_expressionInfo.size() || m_rareData->m_exceptionHandlers.size()));
> 
> What’s the true || doing here?!

This was left there temporarily so as to not perturb the tested code for this change set too much before committing.  This was done deliberately with the intention to fix it in a subsequent change set.  See:
   https://bugs.webkit.org/show_bug.cgi?id=89490