Bug 45358 – DatasetDOMStringMap does not have the right memory model

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

[Erik Arvidsson]

This came up in irc today when we talked about classList DOMTokenList which has the same behavior/issue:

The dataset needs to not fail when the element goes away.

othermaciej: I think the following would work:
[7:06pm] othermaciej: - DOMTokenList is a normal RefCounted
[7:06pm] othermaciej: - Element keeps a ref to it via rare data struct
[7:06pm] othermaciej: - Element destructor presumably already cleans up any rare data that may exist, so rare data can clear out DOMTokenList's backpointer to its element
[7:07pm] othermaciej: - At JS/GC level, Element wrapper keeps DOMTokenList wrapper alive
[7:15pm] arv: jamesr, othermaciej: DatasetDOMStringMap uses the same pattern. I assume that needs to be updated too?
[7:15pm] othermaciej: arv: ask and/or yell at weinig

[Sam Weinig]

Created attachment 66843 [details]
Patch

[Sam Weinig]

I disagree with Maciej that we need to completely alter the memory management of DOMStringMap. The model I used is idiomatic in WebCore. For instance, It is used by CanvasRenderingContext and ValidityState.  That said, I was forgetting to mark the dataset, so I have attached a patch to fix it.

Comment on attachment 66843 [details]
Patch

> +    DOMStringMap* optionalDataset() const;

That’s an awful use of “optional”. r+ only because it’s already being used like this.

[Sam Weinig]

Fixed in r66954. 

Filed https://bugs.webkit.org/show_bug.cgi?id=45367 for a better name.

[Erik Arvidsson]

Comment on attachment 66843 [details]
Patch

This test does not seem correct to me.

Your test seems to test that we get back the same wrapper object?

Don't you want to GC the element and ensure that the dataset does not blow up when the element goes away? (and that the element can be GC'd when there are only references to the dataset)

Is there a reason why you use __proto__ here. Could you have used any property name?

[Sam Weinig]

(In reply to comment #5)
> (From update of attachment 66843 [details])
> This test does not seem correct to me.
> 

Can you elaborate on what is incorrect about it?

> Your test seems to test that we get back the same wrapper object?

Yes.

> 
> Don't you want to GC the element and ensure that the dataset does not blow up when the element goes away? (and that the element can be GC'd when there are only references to the dataset)

I am not entirely sure what you are getting at. Can you provide a test case?

> Is there a reason why you use __proto__ here. Could you have used any property name?

Yeah, I was being sneaky due to the fact that dataset intercepts all property sets except for a few built in properties and puts them in the DOM as attributes, so it wouldn't be testing the right thing?

[Erik Arvidsson]

Would it not be better to tie the lifetime of the dataset to the lifetime of the element it came from?

Also see Bug 45426

[Sam Weinig]

(In reply to comment #7)
> Would it not be better to tie the lifetime of the dataset to the lifetime of the element it came from?
> 
> Also see Bug 45426

That is what is done.

[Erik Arvidsson]

Forgive my ignorance.

I think what is confusing is that markDOMObjectWrapper takes an argument called globalData.

[Eric Seidel (no email)]

Looks like this was landed but just not closed.