Bug 201309 – Assertions in JSArrayBufferView::byteOffset() are only valid for the mutator thread.

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

[Mark Lam]

<rdar://problem/54832121>

[Mark Lam]

Created attachment 377620 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 377620 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSArrayBufferView.h:177
> +    enum ResultQuality { AccurateResult, RacyResult };
> +    template<ResultQuality = AccurateResult> unsigned byteOffset();

This is OK for internal implementation. But I think usually we are using the name `xxxConcurrently()` for this type of function.
So, I think `byteOffsetConcurrently()` would be better.

> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:86
>      ArrayBuffer* buffer = possiblySharedBuffer();

Is it safe to be called from DFG thread? If we go to JSArrayBufferView::slowDownAndWasteMemory, we allocate Butterfly in that function, and it is not allowed in concurrent compilers.

[Mark Lam]

Comment on attachment 377620 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:86
>>      ArrayBuffer* buffer = possiblySharedBuffer();
> 
> Is it safe to be called from DFG thread? If we go to JSArrayBufferView::slowDownAndWasteMemory, we allocate Butterfly in that function, and it is not allowed in concurrent compilers.

Thanks.  I will investigate.

[Mark Lam]

Comment on attachment 377620 [details]
proposed patch.

Take out of review while this is being investigated.

[Mark Lam]

Comment on attachment 377620 [details]
proposed patch.

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

>>> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:86
>>>      ArrayBuffer* buffer = possiblySharedBuffer();
>> 
>> Is it safe to be called from DFG thread? If we go to JSArrayBufferView::slowDownAndWasteMemory, we allocate Butterfly in that function, and it is not allowed in concurrent compilers.
> 
> Thanks.  I will investigate.

Turns out, this is safe because the hasArrayBuffer() check above ensures that mode() is never FastTypedArray or OversizeTypedArray, and hence, we'll never call JSArrayBufferView::slowDownAndWasteMemory().

[Mark Lam]

Created attachment 377929 [details]
proposed patch.

[Mark Lam]

Created attachment 377931 [details]
proposed patch.

[Mark Lam]

Comment on attachment 377931 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91
> +        ASSERT(isMainThread());

Looks like I can't assert this.  Worker threads will not be happy.

[Mark Lam]

Created attachment 377945 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 377931 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91
>> +        ASSERT(isMainThread());
> 
> Looks like I can't assert this.  Worker threads will not be happy.

You can use `ASSERT(!isCompilationThread());` instead.

[Mark Lam]

(In reply to Yusuke Suzuki from comment #10)
> Comment on attachment 377931 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377931&action=review
> 
> >> Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91
> >> +        ASSERT(isMainThread());
> > 
> > Looks like I can't assert this.  Worker threads will not be happy.
> 
> You can use `ASSERT(!isCompilationThread());` instead.

Sounds good.  Fixing.

[Mark Lam]

Created attachment 377947 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 377947 [details]
proposed patch.

r=me

[Saam Barati]

(In reply to Mark Lam from comment #8)
> Comment on attachment 377931 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377931&action=review
> 
> > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91
> > +        ASSERT(isMainThread());
> 
> Looks like I can't assert this.  Worker threads will not be happy.

I think we made this mistake in another patch recently too, right?

[Saam Barati]

(In reply to Saam Barati from comment #14)
> (In reply to Mark Lam from comment #8)
> > Comment on attachment 377931 [details]
> > proposed patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=377931&action=review
> > 
> > > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91
> > > +        ASSERT(isMainThread());
> > 
> > Looks like I can't assert this.  Worker threads will not be happy.
> 
> I think we made this mistake in another patch recently too, right?

https://bugs.webkit.org/show_bug.cgi?id=201281

[Mark Lam]

(In reply to Saam Barati from comment #15)
> (In reply to Saam Barati from comment #14)
> > (In reply to Mark Lam from comment #8)
> > > Comment on attachment 377931 [details]
> > > proposed patch.
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=377931&action=review
> > > 
> > > > Source/JavaScriptCore/runtime/JSArrayBufferViewInlines.h:91
> > > > +        ASSERT(isMainThread());
> > > 
> > > Looks like I can't assert this.  Worker threads will not be happy.
> > 
> > I think we made this mistake in another patch recently too, right?
> 
> https://bugs.webkit.org/show_bug.cgi?id=201281

In 201281, the mistake reduces the strength of the assertion for worker threads instead of overly asserting it (which resulted in crashes) like this case.  Regardless, I'll fix it.

[Mark Lam]

Thanks for the review.  Landed in r249458: <http://trac.webkit.org/r249458>.