There's an "upcast" pointer in the ActiveDOMObject constructor. This was introduced in the original implementation of ActiveDOMObject here (https://bugs.webkit.org/show_bug.cgi?id=21642). There's no real documentation on what it's supposed to be used for, so I have to imagine that the idea is that it holds a pointer to the most-derived class. I guess this was put in with the idea that it was going to be used somewhere, but for the life of me I can't see anywhere that uses it. I've tested removing it on chromium and webkit for mac, and it seems to do fine without it (i.e. the layouttests pass). The reason I want to remove it is I'm trying to make a whole class hierarchy (AudioNodes) into ActiveDOMObjects to solve a GC issue. Having the extra upcast pointer argument means I'd have to thread the argument through the AudioNode constructor and the constructor of every base class derived from it, which doesn't really sound incredibly fun. Is this actually used somewhere, and I just missed it? This is probably the most likely scenario. If it is not used anywhere, does this look like a safe change to you guys?
Created attachment 194149 [details] Patch
Comment on attachment 194149 [details] Patch Does it need to be a hashset? Maybe vector is enough, isn't it? (I cannot imagine a situation where we want to remove active DOM objects from the hash set before a ScriptExecutionContext is destructed.)
> I cannot imagine a situation where we want to remove active DOM objects from the hash set before a ScriptExecutionContext is destructed. When the object itself is destroyed. For example, an XMLHttpRequest object that gets garbage collected after finishing it's network request.
Comment on attachment 194149 [details] Patch Attachment 194149 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17207479
> the idea is that it holds a pointer to the most-derived class. I guess this was put in with the idea that it was going to be used somewhere It used to be necessary for garbage collection, see <http://trac.webkit.org/changeset/37649/trunk/WebCore/bindings/js/JSDOMBinding.cpp>. The reason was of course that an object that had a JS wrapper could have a different address than its ActiveDOMObject part in multiple inheritance scenarios. I _think_ that changes in garbage collection have obsoleted the need. > The reason I want to remove it is I'm trying to make a whole class hierarchy (AudioNodes) into ActiveDOMObjects to solve a GC issue. A general word of caution - ActiveDOMObject has very strict requirements of what can be done from suspend/resume functions. ScriptExecutionContext::suspendActiveDOMObjects() iterates over the HashMap, and if any new objects are created or destroyed during iteration, it's an instant security bug.
> A general word of caution - ActiveDOMObject has very strict requirements of what can be done from suspend/resume functions. ScriptExecutionContext::suspendActiveDOMObjects() iterates over the HashMap, and if any new objects are created or destroyed during iteration, it's an instant security bug. These actually do not need suspend and resume, they're only ActiveDOMObjects so that they can alert the GC that they shouldn't be collected when they have pending activity. There's some precedence for this usage, i.e. AudioContext. Does this make sense? Are there other side-effects to making something an ActiveDOMObject?
Created attachment 194257 [details] Patch
Comment on attachment 194257 [details] Patch Attachment 194257 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17255067
I think that using ActiveDOMObject when not needing suspending is an overkill. These don't scale well, as the whole list is iterated when suspend/resume happens. Regardless, I think that this patch is awesome.
(In reply to comment #9) > I think that using ActiveDOMObject when not needing suspending is an overkill. These don't scale well, as the whole list is iterated when suspend/resume happens. Yeah, I agree. Unfortunately the only alternative right now that I can think of is custom bindings (i.e. the "CustomIsReachable" IDL attribute), and that feels like even more overkill. Maybe it would be good at some future date to abstract out the "pending activity" behavior from the "suspend/resume" behavior,
Created attachment 194277 [details] Patch
> the only alternative right now that I can think of is custom bindings This actually makes more sense to me. Anyway, this discussion is off topic for this bug, I think.
sorry about the efl failures, didn't test that locally. Look good to everyone now?
This patch looks good to me, from a GC perspective. upcastPointer used to be used during GC years ago, but is no longer.
Comment on attachment 194277 [details] Patch Clearing flags on attachment: 194277 Committed r146537: <http://trac.webkit.org/changeset/146537>
All reviewed patches have been landed. Closing bug.