Bug 89410 – JSC needs to record line numbers for exception stack traces

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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.

[Mark Lam]

Reference: <rdar://problem/11561479>.

[Mark Lam]

Created attachment 148229 [details]
Fix

[WebKit Review Bot]

Comment on attachment 148229 [details]
Fix

Rejecting attachment 148229 [details] from commit-queue.

[e-mail] 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.

[Geoffrey Garen]

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.

[Mark Lam]

Created attachment 148241 [details]
Revised with Geoff's feedback applied.

[Geoffrey Garen]

Comment on attachment 148241 [details]
Revised with Geoff's feedback applied.

r=me

[WebKit Review Bot]

Comment on attachment 148241 [details]
Revised with Geoff's feedback applied.

Clearing flags on attachment: 148241

Committed r120676: <http://trac.webkit.org/changeset/120676>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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?!

[Mark Lam]

(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