Bug 213792 – Builtin internal wrapper implementation files wrap static global initialization code with incorrect guards

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Tetsuharu Ohzeki [UTC+9]]

The success rate for https://ews-build.webkit.org/#/builders/10 is down after landing bug 213728, I seem.

Bunch of tests for Stream API has been disabled since before, so I suspect we can disable failures instead of reverting bug 213728
https://trac.webkit.org/browser/webkit/trunk/LayoutTests/platform/win/TestExpectations?rev=263744

[Tetsuharu Ohzeki [UTC+9]]

Created attachment 403195 [details]
Patch

[youenn fablet]

Looking at https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Release%20(Tests)/r263785%20(7081)/streams/reference-implementation/readable-stream-templated-pretty-diff.html, it seems that support of streams API is indeed broken.
We should then probably disable streams API runtime flag (and so maybe FetchAPI as well?) or fix that issue. Brent, Per Arne, any idea of what might be happening.
It does not seem great we have a crash here.

[Fujii Hironori]

The following tests are starting to fail since r263700.
Why did removing ENABLE_STREAMS_API macro make them fail?

streams/readable-stream-default-controller-error.html
streams/readable-stream-default-reader-read.html
streams/readable-stream-error-messages.html
streams/readable-stream-getReader.html
streams/shadowing-getReader.html
streams/streams-public-array-api.html

WinCairo r263699 passed the tests.
https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Release%20(Tests)/r263699%20(7048)/results.html

WinCairo r263700 started to fail.
https://build.webkit.org/results/WinCairo%2064-bit%20WKL%20Release%20(Tests)/r263700%20(7049)/results.html

[Fujii Hironori]

> FAIL Exception thrown: ReferenceError: Can't find private variable: PrivateSymbol.setupReadableStreamDefaultController

What does this error message mean?

[Fujii Hironori]

The latest WinCairo WebKit2 crashes just by openning https://mdn.github.io/dom-examples/streams/simple-pump/
while old WinCairo WebKit2 doesn't crash.

Callstack:

> WebKit2.dll!WebCore::ReadableStream::create(JSC::JSGlobalObject & lexicalGlobalObject={...}, WTF::RefPtr<WebCore::ReadableStreamSource,WTF::DumbPtrTraits<WebCore::ReadableStreamSource>> && source={...}) Line 55	C++
> WebKit2.dll!WebCore::FetchBodyOwner::createReadableStream(JSC::JSGlobalObject & state={...}) Line 333	C++
> WebKit2.dll!WebCore::FetchBodyOwner::readableStream(JSC::JSGlobalObject & state={...}) Line 322	C++
> WebKit2.dll!WebCore::jsFetchResponseBodyGetter(JSC::JSGlobalObject & lexicalGlobalObject={...}, WebCore::JSFetchResponse & thisObject={...}, JSC::ThrowScope & throwScope={...}) Line 530	C++
> WebKit2.dll!WebCore::IDLAttribute<WebCore::JSFetchResponse>::get<&WebCore::jsFetchResponseBodyGetter,3>(JSC::JSGlobalObject & lexicalGlobalObject={...}, __int64 thisValue=1994538944968, const char * attributeName=0x00007ffa909b2d04) Line 69	C++
> WebKit2.dll!WebCore::jsFetchResponseBody(JSC::JSGlobalObject * lexicalGlobalObject=0x000001d063066838, __int64 thisValue=1994538944968, JSC::PropertyName __formal={...}) Line 537	C++
> JavaScriptCore.dll!JSC::PropertySlot::customGetter(JSC::JSGlobalObject * globalObject=0x000001d063066838, JSC::PropertyName propertyName={...}) Line 48	C++
> JavaScriptCore.dll!JSC::PropertySlot::getValue(JSC::JSGlobalObject * globalObject=0x000001d063066838, JSC::PropertyName propertyName={...}) Line 415	C++
> JavaScriptCore.dll!JSC::JSValue::get(JSC::JSGlobalObject * globalObject=0x000001d063066838, JSC::PropertyName propertyName={...}, JSC::PropertySlot & slot={...}) Line 963	C++
> JavaScriptCore.dll!JSC::LLInt::performLLIntGetByID(const JSC::Instruction * pc=0x000001d0630ad2a7, JSC::CodeBlock * codeBlock=0x000001d063c48d10, JSC::JSGlobalObject * globalObject=0x000001d063066838, JSC::JSValue baseValue={...}, const JSC::Identifier & ident={...}, JSC::GetByIdModeMetadata & metadata={...}) Line 751	C++
> JavaScriptCore.dll!llint_slow_path_get_by_id(JSC::CallFrame * callFrame=0x000000d002afe240, const JSC::Instruction * pc=0x000001d0630ad2a7) Line 825	C++
> JavaScriptCore.dll!llint_entry()	Unknown
> (broken call stack under llint_entry due to Bug 199399)

JSC::construct(&lexicalGlobalObject, constructor, constructData, args) returned null.

[Tetsuharu Ohzeki [UTC+9]]

r263700 exposes some code path including virtual functions which were only entered in if ENABLE_STREAMS_API=1.

https://trac.webkit.org/changeset/263700/webkit#file13
https://trac.webkit.org/changeset/263700/webkit#file14

[Fujii Hironori]

Do you mean ENABLE_STREAMS_API was 0 before r263700 for AppleWin and WinCairo ports?

ENABLE_STREAMS_API was 1 even before r263700.
And, the test cases were passing.

[Tetsuharu Ohzeki [UTC+9]]

Ah, Sorry. I'd like to say that "r263700 exposed some code path including virtual functions and branch paths in function, it make a investigate a complex...".

----

BTW, I seem it's better to add the runtime flag.

[Fujii Hironori]

I confirmed WinCairo works fine by reverting r263700.
It doesn't crash (Comment 5) and it passes the tests (Comment 3).
I'm confusing.

[Fujii Hironori]

Created attachment 403431 [details]
generated WebCoreJSBuiltinInternals.cpp of WinCairo port

This is the generated WebCoreJSBuiltinInternals.cpp of WinCairo port.
If ENABLE(WEB_RTC) is not true, Streams API builtins aren't properly initialized.
This seems a generate-js-builtins.py bug.

[Fujii Hironori]

Created attachment 403433 [details]
WIP patch

[Fujii Hironori]

Comment on attachment 403433 [details]
WIP patch

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

> Source/JavaScriptCore/Scripts/wkbuiltins/builtins_generate_internals_wrapper_implementation.py:-153
> -        lines.append(BuiltinsGenerator.wrap_with_guard(" || ".join(guards), self._generate_initialize_static_globals()))

This code was added by r203353.

[Fujii Hironori]

Created attachment 403439 [details]
Patch

[Fujii Hironori]

Comment on attachment 403439 [details]
Patch

Clearing flags on attachment: 403439

Committed r263885: <https://trac.webkit.org/changeset/263885>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.