Bug 160553 – [ES6] Module namespace object should not allow unset IC

• Component: New Bugs
• Status: RESOLVED FIXED

[Yusuke Suzuki]

[ES6] Namespace object should not allow IC

[Yusuke Suzuki]

Created attachment 285334 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 285334 [details]
Patch

I'll do a slight change.

[Yusuke Suzuki]

Created attachment 285402 [details]
Patch

[Saam Barati]

Comment on attachment 285402 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:132
> +        JSModuleRecord::Resolution resolution = moduleRecord->resolveExport(exec, Identifier::fromUid(exec, propertyName.uid()));
> +        ASSERT(resolution.type != JSModuleRecord::Resolution::Type::NotFound && resolution.type != JSModuleRecord::Resolution::Type::Ambiguous);
> +
> +        JSModuleRecord* targetModule = resolution.moduleRecord;
> +        JSModuleEnvironment* targetEnvironment = targetModule->moduleEnvironment();
> +
> +        PropertySlot trampolineSlot(targetEnvironment, PropertySlot::InternalMethodType::Get);
> +        bool found = targetEnvironment->methodTable(exec->vm())->getOwnPropertySlot(targetEnvironment, exec, resolution.localName, trampolineSlot);
> +        ASSERT_UNUSED(found, found);

Why do these assertions have to hold?
I haven't thought too hard, but can't you just call GetOwnProperty on a namespace object and have this not actually be resolved?
like:
`
import * as ns from ...;
Reflect.getOwnPropertyDescriptor(ns, someIdentNotInNS)
`

[Yusuke Suzuki]

Comment on attachment 285402 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:132
>> +        ASSERT_UNUSED(found, found);
> 
> Why do these assertions have to hold?
> I haven't thought too hard, but can't you just call GetOwnProperty on a namespace object and have this not actually be resolved?
> like:
> `
> import * as ns from ...;
> Reflect.getOwnPropertyDescriptor(ns, someIdentNotInNS)
> `

The above example does not reach here because we filter them by

`
if (!thisObject->m_exports.contains(propertyName.uid()))
    return false;
`
in L116.

[Yusuke Suzuki]

Comment on attachment 285402 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:132
>>> +        ASSERT_UNUSED(found, found);
>> 
>> Why do these assertions have to hold?
>> I haven't thought too hard, but can't you just call GetOwnProperty on a namespace object and have this not actually be resolved?
>> like:
>> `
>> import * as ns from ...;
>> Reflect.getOwnPropertyDescriptor(ns, someIdentNotInNS)
>> `
> 
> The above example does not reach here because we filter them by
> 
> `
> if (!thisObject->m_exports.contains(propertyName.uid()))
>     return false;
> `
> in L116.

And this m_exports is collected by JSModuleRecord.cpp's getExportedNames, and already validated by JSModuleRecord::getModuleNamespace.
As a result, this m_exports only contains the names that can be resolved. And when the name is resolved, the resolution always has the target module and the local name.
So looking up the local binding from the target module always succeed.

[Saam Barati]

Comment on attachment 285402 [details]
Patch

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

r=me

>>>> Source/JavaScriptCore/runtime/JSModuleNamespaceObject.cpp:132
>>>> +        ASSERT_UNUSED(found, found);
>>> 
>>> Why do these assertions have to hold?
>>> I haven't thought too hard, but can't you just call GetOwnProperty on a namespace object and have this not actually be resolved?
>>> like:
>>> `
>>> import * as ns from ...;
>>> Reflect.getOwnPropertyDescriptor(ns, someIdentNotInNS)
>>> `
>> 
>> The above example does not reach here because we filter them by
>> 
>> `
>> if (!thisObject->m_exports.contains(propertyName.uid()))
>>     return false;
>> `
>> in L116.
> 
> And this m_exports is collected by JSModuleRecord.cpp's getExportedNames, and already validated by JSModuleRecord::getModuleNamespace.
> As a result, this m_exports only contains the names that can be resolved. And when the name is resolved, the resolution always has the target module and the local name.
> So looking up the local binding from the target module always succeed.

Makes sense.

[Yusuke Suzuki]

Comment on attachment 285402 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 285402 [details]
Patch

Clearing flags on attachment: 285402

Committed r204248: <http://trac.webkit.org/changeset/204248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.