983285 - Add a templated property-value-cleanup function

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

There are a number of places in the code where nsINode::SetProperty is called to set a property on some content node. One of the overloads of this function takes a destruction function that is called to destroy the property value when the property is removed. Many of the callers of the SetProperty function define their own local function which just casts the property value to the concrete type and calls delete on it. As Botond suggested at [1] this could be refactored to a template function somewhere.

Some examples of the kinds of functions that could be rolled into the templated function are [2], [3], [4], [5] and so on. [6] might be a good search to find more of these.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=981029#c14
[2] http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGSVGElement.cpp?rev=8eacfa0523bd#1238
[3] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=44ae8462d6ab#308
[4] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp?rev=44ae8462d6ab#316
[5] http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/APZCCallbackHelper.cpp?rev=509090e51f4c#327
[6] http://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsINode%3A%3ASetProperty%28class+nsIAtom+*%2C+void+*%2C+NSPropertyDtorFunc%2C+_Bool%29%22

Comment 1

[Gorman Ho]

Hi Kartikaya. Can I take this one? I think I know what needs to be done, but this is my first time in the Mozilla codebase so I'll probably need to bother you with newbie questions.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Absolutely. You can follow the instructions at https://developer.mozilla.org/en/docs/Developer_Guide/Build_Instructions to get started with building Firefox locally, and see the instructions at https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch for how to generate a patch with the changes you make.

Feel free to ask me any questions you have either here in the bug or via email. Please note that I'll be away for part of next week though so my responses may be a little delayed during that time.

Comment 3

[Botond Ballo [:botond]]

(In reply to (PTO|Mar15-Mar19) Kartikaya Gupta (email:[e-mail]) from comment #2)
> Feel free to ask me any questions you have either here in the bug or via
> email. Please note that I'll be away for part of next week though so my
> responses may be a little delayed during that time.

Feel free to ask me any questions as well if you can't reach Kats (or in general).

Comment 4

[Gorman Ho]

After exploring the code a bit, here's what I'm thinking. Let me know what you think:

- NSPropertyDtorFunc is a typedef of NSPropertyFunc. NSPropertyFunc is used for both destructing property values and for enumerating property values (nsPropertyTable::Enumerate() and nsPropertyTable::EnumerateAll()). I want to avoid changing NSPropertyFunc, since it'll impact a lot of code unrelated to property value destruction.

In other words, I'm thinking of replacing the NSPropertyDtorFunc typedef with something like:

template<class T>
using NSPropertyDtorFunc = void (*)(void *aObject, nsIAtom *aPropertyName, T *aPropertyValue, void *aData);

There's a bit of duplication with NSPropertyFunc, but I think it's probably a good thing to separate the enumeration callback from the destruction one anyway. We could rename NSPropertyFunc to NSPropertyEnumFunc after this change is made.

I'm on the fence as to whether to replace the existing nsINode::SetProperty() functions with generic ones or whether to just add a new generic version and modify all the places where it can be used. I looked at the code from link [6] in the description and it looks like most, if not all, calls to SetProperty() pass in a destructor. If this is the case, I think I can do the replace safely. I'll confirm this by removing the default value for that parameter and recompiling. This should catch all the trouble areas.

Now some questions:

1. This change seems to have quite a large impact. How would I test these changes?

2. What are nsPropertyTables used for? Is this for storing DOM content?

Thanks for your time and help! Really appreciate it.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to gormanchi from comment #4)
> In other words, I'm thinking of replacing the NSPropertyDtorFunc typedef
> with something like:
> 
> template<class T>
> using NSPropertyDtorFunc = void (*)(void *aObject, nsIAtom *aPropertyName, T
> *aPropertyValue, void *aData);
> 

I think that to start we could do something simpler and more robust. Instead of changing NSPropertyDtorFunc at all, we can just change the call sites to use an instantation of a helper template function that deletes the value. This would reduce the impact of the change, and also allow more flexibility because existing call sites that need to do any other cleanup can be left alone and will work as-is.

> Now some questions:
> 
> 1. This change seems to have quite a large impact. How would I test these
> changes?

If we go with the simpler change, then just successfully compiling and running the code should be enough of a test, since it is basically a mechanical change that the compiler can check.

> 2. What are nsPropertyTables used for? Is this for storing DOM content?

I'm not too sure, but it looks like the DOM uses these to store arbitrary data; for example for the DOM storage feature that is exposed to content.

Comment 6

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (In reply to gormanchi from comment #4)
> > In other words, I'm thinking of replacing the NSPropertyDtorFunc typedef
> > with something like:
> > 
> > template<class T>
> > using NSPropertyDtorFunc = void (*)(void *aObject, nsIAtom *aPropertyName, T
> > *aPropertyValue, void *aData);
> > 
> 
> I think that to start we could do something simpler and more robust. Instead
> of changing NSPropertyDtorFunc at all, we can just change the call sites to
> use an instantation of a helper template function that deletes the value.
> This would reduce the impact of the change, and also allow more flexibility
> because existing call sites that need to do any other cleanup can be left
> alone and will work as-is.

Yeah. This is what I hand in mind when I made the original suggestion at https://bugzilla.mozilla.org/show_bug.cgi?id=981029#c14.

Comment 7

[Gorman Ho]

(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > (In reply to gormanchi from comment #4)
> > > In other words, I'm thinking of replacing the NSPropertyDtorFunc typedef
> > > with something like:
> > > 
> > > template<class T>
> > > using NSPropertyDtorFunc = void (*)(void *aObject, nsIAtom *aPropertyName, T
> > > *aPropertyValue, void *aData);
> > > 
> > 
> > I think that to start we could do something simpler and more robust. Instead
> > of changing NSPropertyDtorFunc at all, we can just change the call sites to
> > use an instantation of a helper template function that deletes the value.
> > This would reduce the impact of the change, and also allow more flexibility
> > because existing call sites that need to do any other cleanup can be left
> > alone and will work as-is.
> 
> Yeah. This is what I hand in mind when I made the original suggestion at
> https://bugzilla.mozilla.org/show_bug.cgi?id=981029#c14.

Got it. I guess I went a step too far! Stay tuned for the patch.

Comment 8

[Gorman Ho]

Attachment: 983285.patch

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8395327 [details] [diff] [review]
983285.patch

Review of attachment 8395327 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This looks great to me, but we should also get a review from a content peer because I don't know all the rules governing that code.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8395327 [details] [diff] [review]
983285.patch

r=me.  Thank you for doing this!

Comment 11

[Gorman Ho]

Is there anything more that I need to do to get this change committed to mozilla-central? I think I've followed all the instructions at [1] so I believe the attached patch is good to go. Please let me know if I need to do anything more. Thanks!

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 12

[Boris Zbarsky [:bzbarsky]]

Just add the "checkin-needed" keyword to the bug.  ;)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/129ef82e5e66

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/129ef82e5e66

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Nice work, Gorman! Feel free to look through the list of mentored bugs and pick up another bug :)