Bug 185438 – Deferred firing of structure transition watchpoints is racy

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

[Michael Saboff]

Currently, the RAII DeferredStructureTransitionWatchpointFire class will fire deferred watchpoints when processing the destructor.  Suppose that another thread is compiling code wants to see that the transition watchpoint has fired, by seeing that they are invalidated.  Given that the compilation occurs on separate threads and the code executing the watchpoint transition may block on GC or for other reasons, the watchpoints won't necessarily have fired.

The watchpoint deferral needs to invalidate the watchpoints and then fire them when able.

[Michael Saboff]

<rdar://problem/39382566>

[Michael Saboff]

Created attachment 339886 [details]
Patch

[Saam Barati]

Comment on attachment 339886 [details]
Patch

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

r=me

Seems reasonable. Can you add a test even if it's racy?

> Source/JavaScriptCore/bytecode/Watchpoint.cpp:101
> +    m_state = IsInvalidated; // Do after moving watchpoints to deferred to transfer original.

I don't follow this comment here. Maybe:
"Do after moving watchpoints to deferredWatchpoints so deferredWatchpoints gets our current state."

That said, we always know state will be IsWatched

> Source/JavaScriptCore/bytecode/Watchpoint.h:332
> +    void fireAll(VM& vm, DeferredWatchpointFire* deferred)
> +    {
> +        if (isFat()) {
> +            fat()->fireAll(vm, deferred);
> +            return;
> +        }
> +        if (decodeState(m_data) == ClearWatchpoint)
> +            return;
> +        m_data = encodeState(IsInvalidated);
> +        WTF::storeStoreFence();
> +    }

Maybe this can be templatized or abstracted since it's almost identical to above fireAll?

> Source/JavaScriptCore/runtime/Structure.cpp:-204
> - 

please revert

[Michael Saboff]

(In reply to Saam Barati from comment #3)
> Comment on attachment 339886 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339886&action=review
> 
> r=me
> 
> I don't follow this comment here. Maybe:
> "Do after moving watchpoints to deferredWatchpoints so deferredWatchpoints
> gets our current state."
> 
> That said, we always know state will be IsWatched

Updated the comment.

> > Source/JavaScriptCore/bytecode/Watchpoint.h:332
> > +    void fireAll(VM& vm, DeferredWatchpointFire* deferred)
> > +    {
> > +        if (isFat()) {
> > +            fat()->fireAll(vm, deferred);
> > +            return;
> > +        }
> > +        if (decodeState(m_data) == ClearWatchpoint)
> > +            return;
> > +        m_data = encodeState(IsInvalidated);
> > +        WTF::storeStoreFence();
> > +    }
> 
> Maybe this can be templatized or abstracted since it's almost identical to
> above fireAll?

Given all the similar fireAll() methods in both this class (InlineWatchpointSet) and WatchpointSet, that refactoring is probably done in another patch.

> > Source/JavaScriptCore/runtime/Structure.cpp:-204
> > - 
> 
> please revert

Done.

[Michael Saboff]

Committed r231518: <https://trac.webkit.org/changeset/231518>

[Michael Catanzaro]

Comment on attachment 339886 [details]
Patch

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

> Source/JavaScriptCore/bytecode/Watchpoint.h:464
> +    JS_EXPORT_PRIVATE DeferredWatchpointFire(VM&);

Should use explicit