Bug 204395 – Web Inspector: Canvas: adjust `InspectorCanvasAgent::recordCanvasAction` to actually check that the `CanvasRenderingContext` still exists

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

The microtask we create inside `InspectorCanvasAgent::recordCanvasAction` to handle marking the end of a frame for a detached canvas doesn't actually check that the `CanvasRenderingContext` still exists, such as if the `CanvasRenderingContext` gets GCd during a recording.

[Devin Rousso]

<rdar://problem/57347504>

[Devin Rousso]

Created attachment 383944 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 383944 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:456
> +        MicrotaskQueue::mainThreadQueue().append(makeUnique<VoidMicrotask>([&, canvasId = inspectorCanvas->identifier()] {
> +            auto inspectorCanvas = m_identifierToInspectorCanvas.get(canvasId);
> +            if (!inspectorCanvas)

How do we know InspectorCanvasAgent is still alive?

[Devin Rousso]

Comment on attachment 383944 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:456
>> +            if (!inspectorCanvas)
> 
> How do we know InspectorCanvasAgent is still alive?

That's a really good point!  Although I'd imagine it's exceedingly rare, I think It is possible for Web Inspector to close before this microtask callback is executed.

[Devin Rousso]

Created attachment 383971 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 383971 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorCanvasAgent.h:169
> +    RecordCanvasActionMicrotask* m_recordCanvasActionMicrotask { nullptr };

Can we just make InspectorCanvasAgent inherit from CanMakeWeakPtr and use that instead?
Storing raw pointer to RecordCanvasActionMicrotask* doesn't seem like a great long term strategy.

[Devin Rousso]

Comment on attachment 383971 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.h:169
>> +    RecordCanvasActionMicrotask* m_recordCanvasActionMicrotask { nullptr };
> 
> Can we just make InspectorCanvasAgent inherit from CanMakeWeakPtr and use that instead?
> Storing raw pointer to RecordCanvasActionMicrotask* doesn't seem like a great long term strategy.

We could do that, but it would require a much larger change, as we use `std::unique_ptr` to hold all of our agents.  I'll investigate shifting everything from `std::unique_ptr` to `Ref`.

[Ryosuke Niwa]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 383971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383971&action=review
> 
> >> Source/WebCore/inspector/agents/InspectorCanvasAgent.h:169
> >> +    RecordCanvasActionMicrotask* m_recordCanvasActionMicrotask { nullptr };
> > 
> > Can we just make InspectorCanvasAgent inherit from CanMakeWeakPtr and use that instead?
> > Storing raw pointer to RecordCanvasActionMicrotask* doesn't seem like a great long term strategy.
> 
> We could do that, but it would require a much larger change, as we use
> `std::unique_ptr` to hold all of our agents.  I'll investigate shifting
> everything from `std::unique_ptr` to `Ref`.

?? WeakPtr is totally compatible with std::unique_ptr.

[Ryosuke Niwa]

e.g. you can do:

class A : public CanMakeWeak<A> {
};

auto a = makeUnique<A>;
auto weakPtrToA = makeWeakPtr(a);
a = nullptr;
weakPtrToA.get() // this will be nullptr here.

[Devin Rousso]

Comment on attachment 383971 [details]
Patch

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

>>>> Source/WebCore/inspector/agents/InspectorCanvasAgent.h:169
>>>> +    RecordCanvasActionMicrotask* m_recordCanvasActionMicrotask { nullptr };
>>> 
>>> Can we just make InspectorCanvasAgent inherit from CanMakeWeakPtr and use that instead?
>>> Storing raw pointer to RecordCanvasActionMicrotask* doesn't seem like a great long term strategy.
>> 
>> We could do that, but it would require a much larger change, as we use `std::unique_ptr` to hold all of our agents.  I'll investigate shifting everything from `std::unique_ptr` to `Ref`.
> 
> ?? WeakPtr is totally compatible with std::unique_ptr.

Interesting.  I was under the (apparently incorrect) assumption that `WeakPtr` required the type to be `RefCounted`.

[Ryosuke Niwa]

(In reply to Devin Rousso from comment #10)
> Comment on attachment 383971 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383971&action=review
> 
> >>>> Source/WebCore/inspector/agents/InspectorCanvasAgent.h:169
> >>>> +    RecordCanvasActionMicrotask* m_recordCanvasActionMicrotask { nullptr };
> >>> 
> >>> Can we just make InspectorCanvasAgent inherit from CanMakeWeakPtr and use that instead?
> >>> Storing raw pointer to RecordCanvasActionMicrotask* doesn't seem like a great long term strategy.
> >> 
> >> We could do that, but it would require a much larger change, as we use `std::unique_ptr` to hold all of our agents.  I'll investigate shifting everything from `std::unique_ptr` to `Ref`.
> > 
> > ?? WeakPtr is totally compatible with std::unique_ptr.
> 
> Interesting.  I was under the (apparently incorrect) assumption that
> `WeakPtr` required the type to be `RefCounted`.

Yeah, it's a common misconception. WeakPtr uses RefPtr as its implementation detail but it's a completely independent feature from RefCounted. Also, Ref/RefPtr doesn't rely on RefCounted either. They just require the object to have ref() and deref() functions. WebCore's Node, for example, doesn't inherit from RefCounted but it's still used with Ref & RefPtr.

[Devin Rousso]

Created attachment 384122 [details]
Patch

[Devin Rousso]

Created attachment 384123 [details]
Patch

Oops.  Small typo.

[Ryosuke Niwa]

Comment on attachment 384123 [details]
Patch

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

The mechanism WeakPtr part of the change looks good but perhaps someone familiar with WebInspector should also review this code change too.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:454
> +            scriptExecutionContext->eventLoop().queueMicrotask([weakThis = makeWeakPtr(*this)] {

This change makes sense to me.

[Timothy Hatcher]

Comment on attachment 384123 [details]
Patch

Looks fine to me.

[Blaze Burg]

Comment on attachment 384123 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 384123 [details]
Patch

Clearing flags on attachment: 384123

Committed r252792: <https://trac.webkit.org/changeset/252792>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.