Bug 112858 – Remove upcastPointer from ActiveDOMObject constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Russell McClellan]

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?

[Russell McClellan]

Created attachment 194149 [details]
Patch

[Kentaro Hara]

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.)

[Adam Barth]

> 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.

[EFL EWS Bot]

Comment on attachment 194149 [details]
Patch

Attachment 194149 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17207479

[Alexey Proskuryakov]

> 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.

[Russell McClellan]

> 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?

[Russell McClellan]

Created attachment 194257 [details]
Patch

[EFL EWS Bot]

Comment on attachment 194257 [details]
Patch

Attachment 194257 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17255067

[Alexey Proskuryakov]

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.

[Russell McClellan]

(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,

[Russell McClellan]

Created attachment 194277 [details]
Patch

[Alexey Proskuryakov]

> 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.

[Russell McClellan]

sorry about the efl failures, didn't test that locally.  Look good to everyone now?

[Geoffrey Garen]

This patch looks good to me, from a GC perspective. upcastPointer used to be used during GC years ago, but is no longer.

[WebKit Review Bot]

Comment on attachment 194277 [details]
Patch

Clearing flags on attachment: 194277

Committed r146537: <http://trac.webkit.org/changeset/146537>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.