Bug 221423 – Potential crash under BaseAudioContext's toJSNewlyCreated()

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Dumez]

Potential crash under BaseAudioContext's toJSNewlyCreated():

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore                   0x000000056f16f61b WTFCrashWithInfo(int, char const*, char const*, int) + 27 (Assertions.h:671)
1   com.apple.WebCore                   0x000000056f73471e WebCore::toJSNewlyCreated(JSC::JSGlobalObject*, WebCore::JSDOMGlobalObject*, WTF::Ref<WebCore::BaseAudioContext, WTF::RawPtrTraits<WebCore::BaseAudioContext> >&&) + 430 (JSBaseAudioContext.cpp:808)
2   com.apple.WebCore                   0x000000056f734903 JSC::JSValue WebCore::wrap<WebCore::BaseAudioContext>(JSC::JSGlobalObject*, WebCore::JSDOMGlobalObject*, WebCore::BaseAudioContext&) + 275 (JSDOMWrapperCache.h:204)
3   com.apple.WebCore                   0x000000056f7347e9 WebCore::toJS(JSC::JSGlobalObject*, WebCore::JSDOMGlobalObject*, WebCore::BaseAudioContext&) + 9 (JSBaseAudioContext.cpp:815)
4   com.apple.WebCore                   0x000000056f7bc9ae WebCore::toJS(JSC::JSGlobalObject*, WebCore::JSDOMGlobalObject*, WebCore::BaseAudioContext*) + 14 (JSBaseAudioContext.h:91)
5   com.apple.WebCore                   0x000000056f7bc993 JSC::JSValue WebCore::JSConverter<WebCore::IDLInterface<WebCore::BaseAudioContext> >::convert<WTF::RefPtr<WebCore::BaseAudioContext, WTF::RawPtrTraits<WebCore::BaseAudioContext>, WTF::DefaultRefDerefTraits<WebCore::BaseAudioContext> > >(JSC::JSGlobalObject&, WebCore::JSDOMGlobalObject&, WTF::RefPtr<WebCore::BaseAudioContext, WTF::RawPtrTraits<WebCore::BaseAudioContext>, WTF::DefaultRefDerefTraits<WebCore::BaseAudioContext> > const&) + 35 (JSDOMConvertInterface.h:81)

We're hitting this assertion:
    // If you hit this assertion you either have a use after free bug, or
    // BaseAudioContext has subclasses. If BaseAudioContext has subclasses that get passed
    // to toJS() we currently require BaseAudioContext you to opt out of binding hardening
    // by adding the SkipVTableValidation attribute to the interface IDL definition
    RELEASE_ASSERT(actualVTablePointer == expectedVTablePointer);

[Chris Dumez]

<rdar://73352543>

[Chris Dumez]

Created attachment 419308 [details]
Patch

[Darin Adler]

Comment on attachment 419308 [details]
Patch

Is there some simpler pattern that is less specific to the design of the classes that can be used to test the "correct wrapper" consistently? I remember when we (I?) did this for a lot of DOM classes with the fast/dom/wrapper-classes.html test, but it seems that was ended up being specific to the DOM than to wrapping in general.

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 419308 [details]
> Patch
> 
> Is there some simpler pattern that is less specific to the design of the
> classes that can be used to test the "correct wrapper" consistently? I
> remember when we (I?) did this for a lot of DOM classes with the
> fast/dom/wrapper-classes.html test, but it seems that was ended up being
> specific to the DOM than to wrapping in general.

I am not aware of a simpler pattern (but I cc'd Sam). I thought this was the usual pattern for such things. In theory though, the bindings generator should be able to generate such code correctly since it knows about the inheritance (and could rely on is<XXX>() type checks).

[Chris Dumez]

Created attachment 419329 [details]
Patch

[EWS]

Committed r272395: <https://trac.webkit.org/changeset/272395>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419329 [details].

[Sam Weinig]

(In reply to Darin Adler from comment #3)
> Comment on attachment 419308 [details]
> Patch
> 
> Is there some simpler pattern that is less specific to the design of the
> classes that can be used to test the "correct wrapper" consistently? I
> remember when we (I?) did this for a lot of DOM classes with the
> fast/dom/wrapper-classes.html test, but it seems that was ended up being
> specific to the DOM than to wrapping in general.

One idea to catch this in testing when adding new types, that might be a bit annoying, but maybe good in the long term, would be to extend fast/dom/wrapper-classes.html to require every type (probably by iterating the exposed constructors, though we might want to do something with internals for the non-exposed constructors) be tested.

I'm thinking something like (js pseudo code):


let setOfAllTypes = makeSetOfAllTypes();

function test(type, expression)
{
    shouldBe("jsWrapperClass(" + expression + ")", "'" + type + "'");
    shouldBe("jsWrapperClass(" + expression + ".__proto__)", "'" + type + "'");
    shouldBe("jsWrapperClass(" + expression + ".constructor)", "'Function'");

    setOfAllTypes.remove(type);
}


...

test("Attr", "document.createAttribute('test')");


...


if (setOfAllTypes.size != 0) {
   for (const type of setOfAllTypes) {
       fail(`Missing type: ${type}`);
   }
}

[Darin Adler]

I love tests like that, but I thought that people really don’t like tests like that because they are hard to keep consistent across platform configurations as features are enabled and disabled.

[Sam Weinig]

(In reply to Darin Adler from comment #8)
> I love tests like that, but I thought that people really don’t like tests
> like that because they are hard to keep consistent across platform
> configurations as features are enabled and disabled.

If by people, you mean Sam Weinig, then you are totally right, but I think we could make this work (famous last words) without that problem by ensuring the output doesn't have a line for each successfully passed type (which would require separate -expected.txt files) and only notes failures. Also, since these are based on constructor types, parts that are conditionally disabled via feature configuration can simply be guarded by "if (window.Foo)" like a real website would.