Bug 198063 – Cleanup Yarr regexp code around paren contexts.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Cleanup Yarr regexp code around paren contexts.

[Keith Miller]

Created attachment 370294 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 370294 [details]
Patch

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

Nice, r=me

> Source/JavaScriptCore/runtime/RegExpInlines.h:118
>      bool m_needBuffer;

Do we need this m_needBuffer field? I think if this is true, m_buffer needs to be non nullptr. And if this is false, m_buffer is nullptr.

> Source/JavaScriptCore/runtime/RegExpInlines.h:119
>      void* m_buffer;

I think adding `{ nullptr }` here is better. Otherwise, when `ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)` is false, this m_buffer is left as uninitialized.
It is OK so long as we don't use it actually. But touching `m_buffer` in `size()` is a bit scary :)

[Yusuke Suzuki]

Comment on attachment 370294 [details]
Patch

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

> Source/JavaScriptCore/yarr/YarrJIT.h:60
> +    // Technically freeParenContext and parenContextSize are only used if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS) is set. Fortunately, all the calling conventions we supporthave caller save argument registers.

supporthave => support have
It is nice if we can ensure this with static_assert.

[EWS Watchlist]

Comment on attachment 370294 [details]
Patch

Attachment 370294 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12243064

New failing tests:
imported/blink/fast/canvas/canvas-clip-stack-persistence.html
http/tests/security/canvas-remote-read-remote-video-redirect.html
fast/shadow-dom/svg-use-href-change-in-shadow-tree.html
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
http/tests/security/contentSecurityPolicy/video-with-http-url-allowed-by-csp-media-src-star.html
imported/blink/fast/canvas/bug382588.html

[EWS Watchlist]

Created attachment 370301 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Keith Miller]

Comment on attachment 370294 [details]
Patch

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

>> Source/JavaScriptCore/runtime/RegExpInlines.h:118
>>      bool m_needBuffer;
> 
> Do we need this m_needBuffer field? I think if this is true, m_buffer needs to be non nullptr. And if this is false, m_buffer is nullptr.

Not really but it's only ever stack allocated. I should add WTF_FORBID_HEAP_ALLOCATION... I'll make that change though.

>> Source/JavaScriptCore/runtime/RegExpInlines.h:119
>>      void* m_buffer;
> 
> I think adding `{ nullptr }` here is better. Otherwise, when `ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS)` is false, this m_buffer is left as uninitialized.
> It is OK so long as we don't use it actually. But touching `m_buffer` in `size()` is a bit scary :)

True will change! I think I accidentally removed that during some cleanup.

>> Source/JavaScriptCore/yarr/YarrJIT.h:60
>> +    // Technically freeParenContext and parenContextSize are only used if ENABLE(YARR_JIT_ALL_PARENS_EXPRESSIONS) is set. Fortunately, all the calling conventions we supporthave caller save argument registers.
> 
> supporthave => support have
> It is nice if we can ensure this with static_assert.

Fixed. I don't know of a way to do that static_assert though. :/

[Keith Miller]

It's weird that EWS says this breaks tests but the bubble is green... and the history there says it passed. O.o

[Keith Miller]

Committed r245586: <https://trac.webkit.org/changeset/245586>

[Radar WebKit Bug Importer]

<rdar://problem/50991840>

[Shawn Roberts]

After changes in https://trac.webkit.org/changeset/245586

OpenSource testers are exiting early with 50 crashes.

First iOS Debug exit:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3806

First Mac Release exit:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/4239

Debug crashes give the following assertion.

ASSERTION FAILED: m_regExpPatternContextLock.isLocked()
./runtime/VM.cpp(958) : void JSC::VM::releaseRegExpPatternContexBuffer()
1   0x5408285e9 WTFCrash
2   0x54082b3ab WTFCrashWithInfo(int, char const*, char const*, int)
3   0x541b84ee4 JSC::VM::releaseRegExpPatternContexBuffer()
4   0x541251e4f JSC::PatternContextBufferHolder::~PatternContextBufferHolder()
5   0x5412515e5 JSC::PatternContextBufferHolder::~PatternContextBufferHolder()
6   0x541adc80e int JSC::RegExp::matchInline<WTF::Vector<int, 0ul, WTF::CrashOnOverflow, 16ul> >(JSC::VM&, WTF::String const&, unsigned int, WTF::Vector<int, 0ul, WTF::CrashOnOverflow, 16ul>&)
7   0x541adc403 JSC::RegExp::match(JSC::VM&, WTF::String const&, unsigned int, WTF::Vector<int, 0ul, WTF::CrashOnOverflow, 16ul>&)
8   0x541b3821a JSC::RegExpGlobalData::performMatch(JSC::VM&, JSC::JSGlobalObject*, JSC::RegExp*, JSC::JSString*, WTF::String const&, int, int**)
9   0x541b2761f JSC::replaceUsingRegExpSearch(JSC::VM&, JSC::ExecState*, JSC::JSString*, JSC::JSValue, JSC::CallData&, JSC::CallType, WTF::String&, JSC::JSValue)
10  0x541b288f4 JSC::replaceUsingRegExpSearch(JSC::VM&, JSC::ExecState*, JSC::JSString*, JSC::JSValue, JSC::JSValue)
11  0x541b2391b JSC::stringProtoFuncReplaceUsingRegExp(JSC::ExecState*)

[Saam Barati]

(In reply to Shawn Roberts from comment #10)
> After changes in https://trac.webkit.org/changeset/245586
> 
> OpenSource testers are exiting early with 50 crashes.
> 
> First iOS Debug exit:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3806
> 
> First Mac Release exit:
> https://build.webkit.org/builders/
> Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/4239
> 
> Debug crashes give the following assertion.
> 
> ASSERTION FAILED: m_regExpPatternContextLock.isLocked()
> ./runtime/VM.cpp(958) : void JSC::VM::releaseRegExpPatternContexBuffer()
> 1   0x5408285e9 WTFCrash
> 2   0x54082b3ab WTFCrashWithInfo(int, char const*, char const*, int)
> 3   0x541b84ee4 JSC::VM::releaseRegExpPatternContexBuffer()
> 4   0x541251e4f
> JSC::PatternContextBufferHolder::~PatternContextBufferHolder()
> 5   0x5412515e5
> JSC::PatternContextBufferHolder::~PatternContextBufferHolder()
> 6   0x541adc80e int JSC::RegExp::matchInline<WTF::Vector<int, 0ul,
> WTF::CrashOnOverflow, 16ul> >(JSC::VM&, WTF::String const&, unsigned int,
> WTF::Vector<int, 0ul, WTF::CrashOnOverflow, 16ul>&)
> 7   0x541adc403 JSC::RegExp::match(JSC::VM&, WTF::String const&, unsigned
> int, WTF::Vector<int, 0ul, WTF::CrashOnOverflow, 16ul>&)
> 8   0x541b3821a JSC::RegExpGlobalData::performMatch(JSC::VM&,
> JSC::JSGlobalObject*, JSC::RegExp*, JSC::JSString*, WTF::String const&, int,
> int**)
> 9   0x541b2761f JSC::replaceUsingRegExpSearch(JSC::VM&, JSC::ExecState*,
> JSC::JSString*, JSC::JSValue, JSC::CallData&, JSC::CallType, WTF::String&,
> JSC::JSValue)
> 10  0x541b288f4 JSC::replaceUsingRegExpSearch(JSC::VM&, JSC::ExecState*,
> JSC::JSString*, JSC::JSValue, JSC::JSValue)
> 11  0x541b2391b JSC::stringProtoFuncReplaceUsingRegExp(JSC::ExecState*)

Is this fixed?

[Yusuke Suzuki]

(In reply to Saam Barati from comment #11)
> (In reply to Shawn Roberts from comment #10)
> > After changes in https://trac.webkit.org/changeset/245586
> > 
> > OpenSource testers are exiting early with 50 crashes.
> > 
> > First iOS Debug exit:
> > https://build.webkit.org/builders/
> > Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29/builds/3806
> > 
> > First Mac Release exit:
> > https://build.webkit.org/builders/
> > Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/4239
> > 
> > Debug crashes give the following assertion.
> > 
> > ASSERTION FAILED: m_regExpPatternContextLock.isLocked()
> > ./runtime/VM.cpp(958) : void JSC::VM::releaseRegExpPatternContexBuffer()
> > 1   0x5408285e9 WTFCrash
> > 2   0x54082b3ab WTFCrashWithInfo(int, char const*, char const*, int)
> > 3   0x541b84ee4 JSC::VM::releaseRegExpPatternContexBuffer()
> > 4   0x541251e4f
> > JSC::PatternContextBufferHolder::~PatternContextBufferHolder()
> > 5   0x5412515e5
> > JSC::PatternContextBufferHolder::~PatternContextBufferHolder()
> > 6   0x541adc80e int JSC::RegExp::matchInline<WTF::Vector<int, 0ul,
> > WTF::CrashOnOverflow, 16ul> >(JSC::VM&, WTF::String const&, unsigned int,
> > WTF::Vector<int, 0ul, WTF::CrashOnOverflow, 16ul>&)
> > 7   0x541adc403 JSC::RegExp::match(JSC::VM&, WTF::String const&, unsigned
> > int, WTF::Vector<int, 0ul, WTF::CrashOnOverflow, 16ul>&)
> > 8   0x541b3821a JSC::RegExpGlobalData::performMatch(JSC::VM&,
> > JSC::JSGlobalObject*, JSC::RegExp*, JSC::JSString*, WTF::String const&, int,
> > int**)
> > 9   0x541b2761f JSC::replaceUsingRegExpSearch(JSC::VM&, JSC::ExecState*,
> > JSC::JSString*, JSC::JSValue, JSC::CallData&, JSC::CallType, WTF::String&,
> > JSC::JSValue)
> > 10  0x541b288f4 JSC::replaceUsingRegExpSearch(JSC::VM&, JSC::ExecState*,
> > JSC::JSString*, JSC::JSValue, JSC::JSValue)
> > 11  0x541b2391b JSC::stringProtoFuncReplaceUsingRegExp(JSC::ExecState*)
> 
> Is this fixed?

Yes, this is fixed in http://trac.webkit.org/changeset/245593/webkit

[Michael Catanzaro]

Note we're considering rolling this out in bug #198518 since some of the assumptions regarding MatchResult and EncodedMatchResult were wrong and the fix is unclear. Would be great to get your help there.