[ES6] Namespace object should not allow IC
Created attachment 285334 [details] Patch
Comment on attachment 285334 [details] Patch I'll do a slight change.
Created attachment 285402 [details] Patch
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) `
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.
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.
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.
Comment on attachment 285402 [details] Patch Thanks!
Comment on attachment 285402 [details] Patch Clearing flags on attachment: 285402 Committed r204248: <http://trac.webkit.org/changeset/204248>
All reviewed patches have been landed. Closing bug.