Bug 59128 – HTMLFormControlElement::labels should allow custom attributes.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yael]

This was introduced in http://trac.webkit.org/changeset/60232.
HTMLFormControlElement::labels() needlessly caches its labels list.

[Yael]

Created attachment 90576 [details]
Patch.

[Sam Weinig]

Comment on attachment 90576 [details]
Patch.

This leaves the node list cache in NodeRareData.  I am also not sure we don't want to cache this value.  Consider the following test.

inputElement.labels.customProperty = 1;
alert(inputElement.labels.customProperty);

That should probably work, but probably won't with this patch or the current code.

[Yael]

(In reply to comment #2)
> (From update of attachment 90576 [details])
> This leaves the node list cache in NodeRareData.  I am also not sure we don't want to cache this value.  Consider the following test.
> 
> inputElement.labels.customProperty = 1;
> alert(inputElement.labels.customProperty);
> 
> That should probably work, but probably won't with this patch or the current code.

Right, I was a bit too quick :)
I'll rework this patch.

[Yael]

Created attachment 90617 [details]
Patch.

Cache the node this time. We do want the test in comment #2 to pass :)
Sam, I did not quite understand your comment on irc about strong pointers and could not catch you afterwards.
If it is still a problem please let me know and I'll address that.

[Alexey Proskuryakov]

Comment on attachment 90617 [details]
Patch.

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

I don't deeply understand what's going one like Sam does, but I've got a few comments.

> LayoutTests/fast/forms/script-tests/labels-custom-property.js:1
> +description('Test that we can set and get custom properties on the labels attribute. ');

Could you please merge these .js and .html files? Keeping them separate causes nothing but difficulties, as you need a second step to get from test to its content.

> LayoutTests/fast/forms/script-tests/labels-custom-property.js:11
> +labels.customProperty = 1;
> +shouldBe('labels.customProperty', '1');

Can you add a gc() call between these? I can't easily find any code marking the JS wrapper.

[Sam Weinig]

Comment on attachment 90617 [details]
Patch.

I think you need to change m_labelsNodeListCache to be a weak pointer for this not leak.

[Yael]

(In reply to comment #6)
> (From update of attachment 90617 [details])
> I think you need to change m_labelsNodeListCache to be a weak pointer for this not leak.

Thank you for the review. I am not sure I know what you mean by "weak pointer" ? I cannot find it in wtf. Do you mean DynamicNodeList* or something else?

[Yael]

(In reply to comment #5)
> (From update of attachment 90617 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90617&action=review
> 
Thank you for your comments.
> I don't deeply understand what's going one like Sam does, but I've got a few comments.
> 
> > LayoutTests/fast/forms/script-tests/labels-custom-property.js:1
> > +description('Test that we can set and get custom properties on the labels attribute. ');
> 
> Could you please merge these .js and .html files? Keeping them separate causes nothing but difficulties, as you need a second step to get from test to its content.
> 
Will do :)

> > LayoutTests/fast/forms/script-tests/labels-custom-property.js:11
> > +labels.customProperty = 1;
> > +shouldBe('labels.customProperty', '1');
> 
> Can you add a gc() call between these? I can't easily find any code marking the JS wrapper.
Will do :)
I'll submit a new patch once the pointer issue is resolved.
Going through the logs, I see that the way we cache the DynamicNodeList wrappers was changed this week.
Please look at r84309 and r83990.

[Yael]

Created attachment 90710 [details]
Patch.

Address comment #5 and commnet #6.

[Yael]

(In reply to comment #9)
> Created an attachment (id=90710) [details]
> Patch.
> 
> Address comment #5 and comment #6.

Verified that the LabelsNodeList is being garbage collected and the C++ object is being deleted.

[Yael]

Created attachment 90711 [details]
Patch.

Resolved conflicts.

[Alexey Proskuryakov]

Wait, it''s not a weak pointer, it's a potentially dangling pointer...

For those following along at home, could you please explain why this pointer will never point to released memory?

[Alexey Proskuryakov]

Comment on attachment 90711 [details]
Patch.

Could you please answer that before committing? If the code is wrong in that respect, the patch would be introducing a security bug.

[Yael]

Created attachment 90731 [details]
Patch.

Change LabelsNodeList::create() to take a pointer Node* instead of PassRefPtr.
Change m_labelsNodeListCache to be of type LabelsNodeList*.

[Yael]

(In reply to comment #12)
> Wait, it''s not a weak pointer, it's a potentially dangling pointer...
> 
> For those following along at home, could you please explain why this pointer will never point to released memory?

The pointer is cleared in the destructor of LabelsListNode.

[WebKit Commit Bot]

Comment on attachment 90731 [details]
Patch.

Clearing flags on attachment: 90731

Committed r84693: <http://trac.webkit.org/changeset/84693>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84693 might have broken SnowLeopard Intel Release (Build)