Bug 177952 – Only add prototypes to the PrototypeMap if they're not already present

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: JavaScriptCore

[Saam Barati]

It's cheaper to check if the thing isn't in the prototype map since allocating a Weak is expensive.

[Saam Barati]

Created attachment 322865 [details]
patch

[Michael Saboff]

Comment on attachment 322865 [details]
patch

r=me

[JF Bastien]

Comment on attachment 322865 [details]
patch

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

> Source/JavaScriptCore/runtime/PrototypeMapInlines.h:48
> +        m_prototypes.set(object, object);

Doesn't this do the search twice? Can you use the result iterator from the search instead?

[Saam Barati]

(In reply to JF Bastien from comment #3)
> Comment on attachment 322865 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322865&action=review
> 
> > Source/JavaScriptCore/runtime/PrototypeMapInlines.h:48
> > +        m_prototypes.set(object, object);
> 
> Doesn't this do the search twice? Can you use the result iterator from the
> search instead?

Yeah, this is the optimal thing to do. I'll add the necessary API to WeakMap to allow this to happen.

[Saam Barati]

Created attachment 322873 [details]
patch for landing

[JF Bastien]

Comment on attachment 322873 [details]
patch for landing

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

r=me

> Source/JavaScriptCore/runtime/PrototypeMapInlines.h:47
> +    auto addResult = m_prototypes.add(object, Weak<JSObject>());

Saam, using auto‽

[Saam Barati]

(In reply to JF Bastien from comment #6)
> Comment on attachment 322873 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322873&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/runtime/PrototypeMapInlines.h:47
> > +    auto addResult = m_prototypes.add(object, Weak<JSObject>());
> 
> Saam, using auto‽

Always for addResult 😎

[WebKit Commit Bot]

Comment on attachment 322873 [details]
patch for landing

Clearing flags on attachment: 322873

Committed r222929: <http://trac.webkit.org/changeset/222929>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34840746>

[Ryan Haddad]

This change caused LayoutTests to exit early with an assertion failure:
https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r222929%20(3378)/results.html

ASSERTION FAILED: addResult.iterator->value.get() == object
./runtime/PrototypeMapInlines.h(51) : void JSC::PrototypeMap::addPrototype(JSC::JSObject *)
1   0x118ffb06d WTFCrash
2   0x117d5009d JSC::PrototypeMap::addPrototype(JSC::JSObject*)
3   0x117f657e0 JSC::ObjectPrototype::finishCreation(JSC::VM&, JSC::JSGlobalObject*)
4   0x117f67ffe JSC::ObjectPrototype::create(JSC::VM&, JSC::JSGlobalObject*, JSC::Structure*)
5   0x117d8bf6d JSC::JSGlobalObject::init(JSC::VM&)
6   0x117daae7c JSC::JSGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*)
7   0x10d1220fa WebCore::JSDOMGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*)
8   0x10d271058 WebCore::JSDOMWindowBase::finishCreation(JSC::VM&, WebCore::JSDOMWindowProxy*)
9   0x10d19f9b6 WebCore::JSDOMWindow::finishCreation(JSC::VM&, WebCore::JSDOMWindowProxy*)
10  0x10d27a664 WebCore::JSDOMWindow::create(JSC::VM&, JSC::Structure*, WTF::Ref<WebCore::DOMWindow>&&, WebCore::JSDOMWindowProxy*)
11  0x10d27a127 WebCore::JSDOMWindowProxy::setWindow(WTF::RefPtr<WebCore::DOMWindow>&&)
12  0x10e48d30e WebCore::ScriptController::setDOMWindowForWindowProxy(WebCore::DOMWindow*)
13  0x10c8a5c05 WebCore::FrameLoader::clear(WebCore::Document*, bool, bool, bool)
14  0x10c57a739 WebCore::DocumentWriter::begin(WebCore::URL const&, bool, WebCore::Document*)
15  0x10c530f8e WebCore::DocumentLoader::commitData(char const*, unsigned long)
16  0x10c530a25 WebCore::DocumentLoader::finishedLoading()
17  0x10c537e25 WebCore::DocumentLoader::maybeLoadEmpty()
18  0x10c537fad WebCore::DocumentLoader::startLoadingMainResource()
19  0x10c8c0165 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)::$_7::operator()() const
20  0x10c8bfeb9 WTF::Function<void ()>::CallableWrapper<WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)::$_7>::call()
21  0x10bd9d9db WTF::Function<void ()>::operator()() const
22  0x10c8ac80d WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)
23  0x10c8be9e8 WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL)::$_5::operator()(WebCore::ResourceRequest const&, WebCore::FormState*, bool) const
24  0x10c8be972 WTF::Function<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>::CallableWrapper<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL)::$_5>::call(WebCore::ResourceRequest const&, WebCore::FormState*, bool)
25  0x10dfd3bad WTF::Function<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest const&, WebCore::FormState*, bool) const
26  0x10dfd2bc9 WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest const&, WebCore::FormState*, bool)
27  0x10dfd60b9 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>&&)::$_1::operator()(WebCore::PolicyAction)
28  0x10dfd5c8a WTF::Function<void (WebCore::PolicyAction)>::CallableWrapper<WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>&&)::$_1>::call(WebCore::PolicyAction)
29  0x1055d10b1 WTF::Function<void (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const
30  0x105cf69c5 WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&, WebCore::ResourceRequest const&, bool, WebCore::FormState*, WTF::Function<void (WebCore::PolicyAction)>&&)
31  0x10dfd2a3a WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, bool, WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void (WebCore::ResourceRequest const&, WebCore::FormState*, bool)>&&)
LEAK: 1 WebPageProxy

[Saam Barati]

(In reply to Ryan Haddad from comment #11)
> This change caused LayoutTests to exit early with an assertion failure:
> https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/
> r222929%20(3378)/results.html
> 
> ASSERTION FAILED: addResult.iterator->value.get() == object
> ./runtime/PrototypeMapInlines.h(51) : void
> JSC::PrototypeMap::addPrototype(JSC::JSObject *)
> 1   0x118ffb06d WTFCrash
> 2   0x117d5009d JSC::PrototypeMap::addPrototype(JSC::JSObject*)
> 3   0x117f657e0 JSC::ObjectPrototype::finishCreation(JSC::VM&,
> JSC::JSGlobalObject*)
> 4   0x117f67ffe JSC::ObjectPrototype::create(JSC::VM&, JSC::JSGlobalObject*,
> JSC::Structure*)
> 5   0x117d8bf6d JSC::JSGlobalObject::init(JSC::VM&)
> 6   0x117daae7c JSC::JSGlobalObject::finishCreation(JSC::VM&, JSC::JSObject*)
> 7   0x10d1220fa WebCore::JSDOMGlobalObject::finishCreation(JSC::VM&,
> JSC::JSObject*)
> 8   0x10d271058 WebCore::JSDOMWindowBase::finishCreation(JSC::VM&,
> WebCore::JSDOMWindowProxy*)
> 9   0x10d19f9b6 WebCore::JSDOMWindow::finishCreation(JSC::VM&,
> WebCore::JSDOMWindowProxy*)
> 10  0x10d27a664 WebCore::JSDOMWindow::create(JSC::VM&, JSC::Structure*,
> WTF::Ref<WebCore::DOMWindow>&&, WebCore::JSDOMWindowProxy*)
> 11  0x10d27a127
> WebCore::JSDOMWindowProxy::setWindow(WTF::RefPtr<WebCore::DOMWindow>&&)
> 12  0x10e48d30e
> WebCore::ScriptController::setDOMWindowForWindowProxy(WebCore::DOMWindow*)
> 13  0x10c8a5c05 WebCore::FrameLoader::clear(WebCore::Document*, bool, bool,
> bool)
> 14  0x10c57a739 WebCore::DocumentWriter::begin(WebCore::URL const&, bool,
> WebCore::Document*)
> 15  0x10c530f8e WebCore::DocumentLoader::commitData(char const*, unsigned
> long)
> 16  0x10c530a25 WebCore::DocumentLoader::finishedLoading()
> 17  0x10c537e25 WebCore::DocumentLoader::maybeLoadEmpty()
> 18  0x10c537fad WebCore::DocumentLoader::startLoadingMainResource()
> 19  0x10c8c0165
> WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::
> ResourceRequest const&, WebCore::FormState*, bool,
> WebCore::AllowNavigationToInvalidURL)::$_7::operator()() const
> 20  0x10c8bfeb9 WTF::Function<void
> ()>::CallableWrapper<WebCore::FrameLoader::
> continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&,
> WebCore::FormState*, bool,
> WebCore::AllowNavigationToInvalidURL)::$_7>::call()
> 21  0x10bd9d9db WTF::Function<void ()>::operator()() const
> 22  0x10c8ac80d
> WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::
> ResourceRequest const&, WebCore::FormState*, bool,
> WebCore::AllowNavigationToInvalidURL)
> 23  0x10c8be9e8
> WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*,
> WebCore::FrameLoadType, WebCore::FormState*,
> WebCore::AllowNavigationToInvalidURL)::$_5::operator()(WebCore::
> ResourceRequest const&, WebCore::FormState*, bool) const
> 24  0x10c8be972 WTF::Function<void (WebCore::ResourceRequest const&,
> WebCore::FormState*,
> bool)>::CallableWrapper<WebCore::FrameLoader::loadWithDocumentLoader(WebCore:
> :DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*,
> WebCore::AllowNavigationToInvalidURL)::$_5>::call(WebCore::ResourceRequest
> const&, WebCore::FormState*, bool)
> 25  0x10dfd3bad WTF::Function<void (WebCore::ResourceRequest const&,
> WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest const&,
> WebCore::FormState*, bool) const
> 26  0x10dfd2bc9 WTF::CompletionHandler<void (WebCore::ResourceRequest
> const&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest
> const&, WebCore::FormState*, bool)
> 27  0x10dfd60b9
> WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest
> const&, bool, WebCore::DocumentLoader*, WebCore::FormState*,
> WTF::CompletionHandler<void (WebCore::ResourceRequest const&,
> WebCore::FormState*, bool)>&&)::$_1::operator()(WebCore::PolicyAction)
> 28  0x10dfd5c8a WTF::Function<void
> (WebCore::PolicyAction)>::CallableWrapper<WebCore::PolicyChecker::
> checkNavigationPolicy(WebCore::ResourceRequest const&, bool,
> WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void
> (WebCore::ResourceRequest const&, WebCore::FormState*,
> bool)>&&)::$_1>::call(WebCore::PolicyAction)
> 29  0x1055d10b1 WTF::Function<void
> (WebCore::PolicyAction)>::operator()(WebCore::PolicyAction) const
> 30  0x105cf69c5
> WebKit::WebFrameLoaderClient::
> dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&,
> WebCore::ResourceRequest const&, bool, WebCore::FormState*,
> WTF::Function<void (WebCore::PolicyAction)>&&)
> 31  0x10dfd2a3a
> WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest
> const&, bool, WebCore::DocumentLoader*, WebCore::FormState*,
> WTF::CompletionHandler<void (WebCore::ResourceRequest const&,
> WebCore::FormState*, bool)>&&)
> LEAK: 1 WebPageProxy

Looking into it. This is really surprising.

[Saam Barati]

This is the bug:

```
void Heap::pruneStaleEntriesFromWeakGCMaps()
{
    if (m_collectionScope != CollectionScope::Full)
        return;
    for (auto& pruneCallback : m_weakGCMaps.values())
        pruneCallback();
}
```
But the WeakGCMap we use is:
WeakGCMap<JSObject*, JSObject*>

which really is
HashMap<JSObject*, Weak<JSObject>>

It's really suspicious we don't clear entries in this map on every GC. I'm working on a more fundamental fix in:
https://bugs.webkit.org/show_bug.cgi?id=177907

[Ryan Haddad]

Reverted r222929 for reason:

Caused assertion failures during LayoutTests.

Committed r222939: <http://trac.webkit.org/changeset/222939>

[Saam Barati]

*** This bug has been marked as a duplicate of bug 177907 ***

[Filip Pizlo]

(In reply to Saam Barati from comment #13)
> This is the bug:
> 
> ```
> void Heap::pruneStaleEntriesFromWeakGCMaps()
> {
>     if (m_collectionScope != CollectionScope::Full)
>         return;
>     for (auto& pruneCallback : m_weakGCMaps.values())
>         pruneCallback();
> }
> ```
> But the WeakGCMap we use is:
> WeakGCMap<JSObject*, JSObject*>
> 
> which really is
> HashMap<JSObject*, Weak<JSObject>>
> 
> It's really suspicious we don't clear entries in this map on every GC. I'm
> working on a more fundamental fix in:
> https://bugs.webkit.org/show_bug.cgi?id=177907

No, your patch was wrong. You should have said:

    auto addResult = m_prototypes.add(object, Weak<JSObject>());
    if (addResult.isNewEntry || !addResult.iterator->value)
        addResult.iterator->value = Weak<JSObject>(object);
    else
        ASSERT(addResult.iterator->value.get() == object);

Pruning a WeakGCMap is an optimization. All methods in WeakGCMap must assume that an entry with a null value is as if the entry was not there at all.

[Filip Pizlo]

Comment on attachment 322873 [details]
patch for landing

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

> Source/JavaScriptCore/runtime/PrototypeMapInlines.h:48
> +    if (addResult.isNewEntry)

This should have also checked if the value is null.

[Filip Pizlo]

I'll fix this in https://bugs.webkit.org/show_bug.cgi?id=178051