[webkit-reviews] review denied: [Bug 59072] LEAK: SVGElement leaks when detaching it in a pending resource state : [Attachment 91254] Patch v3

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 59072: LEAK: SVGElement leaks when detaching it in a pending resource state
https://bugs.webkit.org/show_bug.cgi?id=59072

Attachment 91254: Patch v3
https://bugs.webkit.org/attachment.cgi?id=91254&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91254&action=review

Mainly r- because of the wrong style of the tests. Still some open questions on
the patch itself.

> Source/WebCore/svg/SVGDocumentExtensions.cpp:251
> +    HashMap<AtomicString, SVGPendingElements*>::iterator end =
m_pendingResources.end();

Why can we have multiple resources for one id? Even if the user sets the same
id to two different elements, we don't end up with two resources. What am I
missing? Ah you try to catch the scenario where the user removes one of the
elements and we take the other resource?

>> Source/WebCore/svg/SVGElementRareData.h:64
>> +	bool isPendingResource() const { return m_isPendingResource; }
> 
> Is hasPendingResource() better?

First unofficial reviews. Great! really.

>> Source/WebCore/svg/SVGElementRareData.h:65
>> +	void setIsPendingResource(bool value) { m_isPendingResource = value; }
> 
> Is setHasPendingResource() better?

The naming is correct here. The affected element _is_ a pending resource. But
it may have it self pending resources of course.

>> Source/WebCore/svg/SVGElementRareData.h:78
>> +	bool m_isPendingResource : 1;
> 
> ditto: m_hasPendingResource?

Ditto.

>> Source/WebCore/svg/SVGUseElement.cpp:176
>> +		}
> 
> I can't understand why this <use>'s href affects the other <use>'s. Shall we
call
document()->accessSVGExtensions()->removeElementFromPendingResources(this)?

the use element can have pending resources it self: xlink:hreaf="foo". We need
to remove foo from the pending lists. Nevertheless I agree. Why are you doing
it if the use element is itself a pending resource? Should this if condition
get removed?

> LayoutTests/svg/custom/pending-resource-leak-2.svg:6
> +			layoutTestController.waitUntilDone();

No tabs please. For some reason this is not checked by the style bot :-(

> LayoutTests/svg/custom/pending-resource-leak-3.svg:6
> +			layoutTestController.waitUntilDone();

ditto. The following files have the same problem.