Bug 100347 – A mistake in WebCore::JavaScriptCallFrame::evaluate which will cause assert failed

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

[Peter Wang]

When WebCore::JavaScriptCallFrame::evaluate is invoked in worker context, this statement
      JSLockHolder lock(JSDOMWindowBase::commonJSGlobalData());
will cause assert, since there is "ASSERT(isMainThread());" in JSDOMWindowBase::commonJSGlobalData.

[Peter Wang]

Created attachment 170581 [details]
Patch

[Peter Wang]

it comes from <http://trac.webkit.org/changeset/121381>, a small neglect.

[Mark Lam]

Comment on attachment 170581 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new test cae.

Typo in this line ("cae" ==> "case").  Just delete the line as it doesn't add any info.

Otherwise, LGTM, but need r+ from someone else.

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 170581 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170581&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new test cae.
> 
> Typo in this line ("cae" ==> "case").  Just delete the line as it doesn't add any info.

In WebCore changelogs we usually have a line about test cases. Peter, please fix the typo before landing. My preference would be to keep this line. 

> 
> Otherwise, LGTM, but need r+ from someone else.

[Peter Wang]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 170581 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=170581&action=review
> > 
> > > Source/WebCore/ChangeLog:11
> > > +        No new test cae.
> > 
> > Typo in this line ("cae" ==> "case").  Just delete the line as it doesn't add any info.
> 
> In WebCore changelogs we usually have a line about test cases. Peter, please fix the typo before landing. My preference would be to keep this line. 
> 
> > 
> > Otherwise, LGTM, but need r+ from someone else.

Sorry, thx.

[Peter Wang]

Created attachment 170781 [details]
Patch

[Peter Wang]

Comment on attachment 170781 [details]
Patch

correct a mistake in ChangeLog according comment#5 and land this patch.

[WebKit Review Bot]

Comment on attachment 170781 [details]
Patch

Rejecting attachment 170781 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

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

- If you have reviewer 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 reviewer rights.

[WebKit Review Bot]

Comment on attachment 170781 [details]
Patch

Rejecting attachment 170781 [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.

[WebKit Review Bot]

Comment on attachment 170781 [details]
Patch

Clearing flags on attachment: 170781

Committed r132573: <http://trac.webkit.org/changeset/132573>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Moving all JavaScriptGlue bugs to JavaScriptCore. The JavaScriptGlue framework itself is long gone. And most of the more recent bugs put in this component were put there by people who thought this was for some other aspect of “JavaScript glue” and have nothing to do with the actual original reason for the existence of this component, which was an OS-X-only framework named JavaScriptGlue.