Bug 135358 – Create a more generic way for VMEntryScope to notify those interested that it will be destroyed

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

Currently, when VMEntryScope is destroyed, and it has a flag set that the VM needs recompilation, it calls Debugger::recompileAllJSFunctions.
This is tailored specifically for the Debugger and its purposes of recompiling all functions once the VM stops executing code. 
This patch will substitute this one recompilation flag with a list of callbacks that can all be notified when the VMEntryScope dies. 
This creates a more generalized interface to being notified when VMEntryScope dies.

[Saam Barati]

Created attachment 235630 [details]
patch 

Currently, when VMEntryScope is destroyed, and it has a flag set that the VM
needs recompilation, it calls Debugger::recompileAllJSFunctions. This is
tailored specifically for the Debugger and its purposes of recompiling all
functions once the VM stops executing code (specifically, to ensure that there are 
no functions on the stack that will be clobbered by recompiling them). This patch 
will substitute this one recompilation flag with a list of callbacks that are all 
notified when the VMEntryScope dies. This creates a more generalized interface to 
being notified when the VM stops executing code via the event of VMEntryScope
dying.

[WebKit Commit Bot]

Attachment 235630 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/debugger/Debugger.cpp:338:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/debugger/Debugger.cpp:341:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 235635 [details]
patch 

Same patch as above, but with WebKit style compliant changes.

[Geoffrey Garen]

Comment on attachment 235635 [details]
patch 

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

> Source/JavaScriptCore/debugger/Debugger.cpp:338
> +        VMEntryScope::EntryScopeDidPopListener callback = [this] (VM* vm, JSGlobalObject* globalObject) 

This is probably a good place to use "auto".

> Source/JavaScriptCore/debugger/Debugger.cpp:341
> +            if (globalObject->debugger() && globalObject->debugger() == this)
> +                this->recompileAllJSFunctions(vm);

It feels weird to use 'this' here, since we can't guarantee its lifetime. I think it would be better to use globalObject->debugger() instead of 'this'.

[Mark Lam]

Comment on attachment 235630 [details]
patch 

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

> Source/JavaScriptCore/ChangeLog:10
> +        tailored specifically for the Debugger and its purposes of recompiling all

typos in "its purposes of recompiling"?  Can it be phrased better?

> Source/JavaScriptCore/ChangeLog:14
> +        notified when the VMEntryScope dies. This creates a more generalized interface to 

... when the *outermost* VMEntryScope *exits* ...

> Source/JavaScriptCore/debugger/Debugger.cpp:341
> +            if (globalObject->debugger() && globalObject->debugger() == this) {
> +                this->recompileAllJSFunctions(vm);
> +            }

no need for { }

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:71
> +        callback(&m_vm, m_globalObject);

nit: why not us a VM& instead of a VM*?

> Source/JavaScriptCore/runtime/VMEntryScope.h:47
> +    void addEntryScopeDidPopListener(EntryScopeDidPopListener lambda);

The "lambda" is not needed.  Please remove.

[Saam Barati]

Created attachment 235648 [details]
patch 

Took into account the changes that both Mark and Geoff recommended.

[Geoffrey Garen]

Comment on attachment 235648 [details]
patch 

I think you re-uploaded the old patch.

[Mark Lam]

Comment on attachment 235648 [details]
patch 

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

> Source/JavaScriptCore/ChangeLog:14
> +        to being notified when the VM stops executing code via the event of the outermost 

"to being" ==> "for being"

> Source/JavaScriptCore/debugger/Debugger.cpp:345
> +        auto listener = [this] (VM* vm, JSGlobalObject* globalObject) 
> +        {
> +            Debugger* debugger = globalObject->debugger();
> +            if (debugger && debugger == this)
> +                debugger->recompileAllJSFunctions(vm);
> +        };
> +
> +        vm->entryScope->addEntryScopeDidPopListener(listener);

I just thought of an issue.  This code registers the listener, but you don't have any code to unregister it when the debugger is detached.  Please fix.

[Saam Barati]

(In reply to comment #7)
> (From update of attachment 235648 [details])
> I think you re-uploaded the old patch.

No, this was the updated patch. But I think I took your point yesterday as being that we should call this method on globalObject->debugger and still compare that to 'this', but maybe, you meant that we shouldn't compare to 'this' at all?

So:
Debugger* debugger = globalObject->debugger();
> +            if (debugger && debugger == this)
> +                debugger->recompileAllJSFunctions(vm);

==> becomes ==>

> +            if (debugger)
> +                debugger->recompileAllJSFunctions(vm);


Is that what you meant?

[Saam Barati]

(In reply to comment #8)
> (From update of attachment 235648 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235648&action=review
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        to being notified when the VM stops executing code via the event of the outermost 
> 
> "to being" ==> "for being"
> 
> > Source/JavaScriptCore/debugger/Debugger.cpp:345
> > +        auto listener = [this] (VM* vm, JSGlobalObject* globalObject) 
> > +        {
> > +            Debugger* debugger = globalObject->debugger();
> > +            if (debugger && debugger == this)
> > +                debugger->recompileAllJSFunctions(vm);
> > +        };
> > +
> > +        vm->entryScope->addEntryScopeDidPopListener(listener);
> 
> I just thought of an issue.  This code registers the listener, but you don't have any code to unregister it when the debugger is detached.  Please fix.

I'll create an interface for this.

[Geoffrey Garen]

> So:
> Debugger* debugger = globalObject->debugger();
> > +            if (debugger && debugger == this)
> > +                debugger->recompileAllJSFunctions(vm);
> 
> ==> becomes ==>
> 
> > +            if (debugger)
> > +                debugger->recompileAllJSFunctions(vm);

That's right.

It's not good to use 'this' unless we can guarantee that the object pointed to by 'this' has not been deleted.

It's not good to use a potentially deleted pointer -- even if you never dereference it. If the object pointed to by 'this' is deleted, and a new object is allocated in its place, then our "== this" comparison means something totally different, and probably wrong.

[Saam Barati]

Created attachment 235711 [details]
patch 

More review considerations taken into account. Ultimately, I decided not to provide an interface for unregistering listeners.

If such an interface becomes required, it can be added in the future.

[WebKit Commit Bot]

Attachment 235711 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VMEntryScope.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 235714 [details]
patch 

Fixed out of alphabetical order include.

[Geoffrey Garen]

Comment on attachment 235714 [details]
patch 

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

r=me

> Source/JavaScriptCore/debugger/Debugger.cpp:342
> +            Debugger* debugger = globalObject->debugger();
> +            if (debugger)
> +                debugger->recompileAllJSFunctions(vm);

It's nice to write this as a single line. That way, it's lexically impossible to use the NULL pointer:

if (Debugger* debugger = globalObject->debugger()) { ... }

[WebKit Commit Bot]

Comment on attachment 235714 [details]
patch 

Clearing flags on attachment: 235714

Committed r172009: <http://trac.webkit.org/changeset/172009>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 135627

[Saam Barati]

Created attachment 236068 [details]
patch

Same patch as before, but with Geoff's recommendation taken into account.

[Darin Adler]

Comment on attachment 236068 [details]
patch

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

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:59
> +    m_allEntryScopeDidPopListeners.set(key, listener);

Probably should be add instead of set; set is a slower version that knows how to override values already in the map, and I think the intent here is not to do that.

Also, I think we should assert that they key isn’t already in there.

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:74
> +    auto iter = m_allEntryScopeDidPopListeners.begin();
> +    auto end = m_allEntryScopeDidPopListeners.end();
> +    for ( ; iter != end; ++iter) {
> +        EntryScopeDidPopListener listener = iter->value;
> +        listener(&m_vm, m_globalObject);
>      }

The typical way of writing this kind of thing in new WebKit code is:

    for (auto& listener : m_allEntryScopeDidPopListeners.values())
        listener(&m_vm, m_globalObject);

I think that’s easier to read than the 6-line version.

> Source/JavaScriptCore/runtime/VMEntryScope.h:46
> +    typedef std::function<void (VM*, JSGlobalObject*)> EntryScopeDidPopListener;

Argument should be VM& instead of VM*.

> Source/JavaScriptCore/runtime/VMEntryScope.h:52
> +    HashMap<void*, EntryScopeDidPopListener> m_allEntryScopeDidPopListeners;

Is void* really the best type for these keys?

Why does this need to be a map? Can’t it just be a Vector? Do we ever remove or overwrite any of these?

[Saam Barati]

(In reply to comment #20)
> (From update of attachment 236068 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236068&action=review
> 
> > Source/JavaScriptCore/runtime/VMEntryScope.cpp:59
> > +    m_allEntryScopeDidPopListeners.set(key, listener);
> 
> Probably should be add instead of set; set is a slower version that knows how to override values already in the map, and I think the intent here is not to do that.
> 
> Also, I think we should assert that they key isn’t already in there.
> 
The intention here is to override the current listener if there is one. Those interested should be able to register listeners as many times as needed without having to worry handling duplicate callbacks themselves. This use case is currently unused because the only users of this API are various WebInspector Objects that subclass Debugger, who only register for this callback once, when they themselves attach to their first global object and request that all JS functions are recompiled. But I have another patch in the works that uses this feature to re-register callbacks (https://bugs.webkit.org/show_bug.cgi?id=135423). That being said, this method is named badly. I think it should be changed from "addEntryScopeDidPopListener" to "setEntryScopeDidPopListener" to better indicate its behavior.

> > Source/JavaScriptCore/runtime/VMEntryScope.cpp:74
> > +    auto iter = m_allEntryScopeDidPopListeners.begin();
> > +    auto end = m_allEntryScopeDidPopListeners.end();
> > +    for ( ; iter != end; ++iter) {
> > +        EntryScopeDidPopListener listener = iter->value;
> > +        listener(&m_vm, m_globalObject);
> >      }
> 
> The typical way of writing this kind of thing in new WebKit code is:
> 
>     for (auto& listener : m_allEntryScopeDidPopListeners.values())
>         listener(&m_vm, m_globalObject);
> 
> I think that’s easier to read than the 6-line version.
> 
I agree, this is much nicer.

> > Source/JavaScriptCore/runtime/VMEntryScope.h:46
> > +    typedef std::function<void (VM*, JSGlobalObject*)> EntryScopeDidPopListener;
> 
> Argument should be VM& instead of VM*.
Okay.

> 
> > Source/JavaScriptCore/runtime/VMEntryScope.h:52
> > +    HashMap<void*, EntryScopeDidPopListener> m_allEntryScopeDidPopListeners;
> 
> Is void* really the best type for these keys?
The intention of this API is for the Objects that are interested in receiving this notification to pass themselves in as a pointer to uniquely identify themselves. So I think void* is the best way to describe that any object might request this notification and that object uses itself as its identification. How would you recommend describing this behavior?

> 
> Why does this need to be a map? Can’t it just be a Vector? Do we ever remove or overwrite any of these?
For the reason stated above that we do overwrite old values on conflict (which will be utilized the the above patch I mentioned) and do uniquing based on pointers being passed in.

[Saam Barati]

Created attachment 236245 [details]
patch

Above comments taken into consideration.

[WebKit Commit Bot]

Comment on attachment 236245 [details]
patch

Rejecting attachment 236245 [details] from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Commit Bot]

Comment on attachment 236245 [details]
patch

Clearing flags on attachment: 236245

Committed r172324: <http://trac.webkit.org/changeset/172324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.