Web Inspector: Canvas: unbinding a canvas should always remove the agent as an observer

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

`InspectorCanvasAgent::unbindCanvas` is currently called in two places:
 - `InspectorCanvasAgent::frameNavigated`, right after the `InspectorCanvasAgent` removes itself as an observer from the canvas in question
 - `InspectorCanvasAgent::canvasDestroyed`, which is called on all observers when the canvas is about to be destructed
Previously, in order to avoid modification-while-iterating, we wouldn't remove the `InspectorCanvasAgent` from the canvas inside `InspectorCanvasAgent::canvasDestroyed`, since the canvas is about to be destructed anyways.  This isn't a very good practice, and should be "corrected" (e.g. the observer shouldn't have any idea about the specifics of how the canvas treats/notifies its observers).

[Radar WebKit Bug Importer]

<rdar://problem/49357109>

[Devin Rousso]

Created attachment 366119 [details]
Patch

[Matt Baker]

Comment on attachment 366119 [details]
Patch

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

r=me

> Source/WebCore/html/CanvasBase.cpp:74
>          observer->canvasChanged(*this, rect);

I noticed this is a widely used pattern in WebKit. Is it to safeguard against m_observers mutating during iteration?

[Devin Rousso]

Comment on attachment 366119 [details]
Patch

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

>> Source/WebCore/html/CanvasBase.cpp:74
>>          observer->canvasChanged(*this, rect);
> 
> I noticed this is a widely used pattern in WebKit. Is it to safeguard against m_observers mutating during iteration?

Yup!

[WebKit Commit Bot]

Comment on attachment 366119 [details]
Patch

Clearing flags on attachment: 366119

Committed r243611: <https://trac.webkit.org/changeset/243611>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.