Cleanup ContentDOMReference a bit
Categories
(Toolkit :: General, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox69 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
Details
Attachments
(3 files)
There are a few issues with ContentDOMReference which immediately stand out to me. I'm only going to deal with two of them here:
-
It uses UUIDs for the element IDs. There's really no reason for this. The UUID generator is essentially just a relatively expensive random-number generator, and since the IDs that it generates as strings, they're much more expensive to use as map keys (since they have to be atomized for every lookup) and to transfer between processes. Using Math.random() directly is more than sufficient. Or, for that matter, just using a counter that we increment on each use.
-
It relies on callers to explicitly destroy every ID that they create in order to avoid leaking mapping entries. This isn't great, and it's something we can fairly easily avoid using the finalization witness service.
And, though I don't intend to address it here, and related to point #2,
- If multiple callers create references to the same element at the same time, the first one to revoke its reference will also revoke the references being used by other callers. I'm not sure how likely this is to happen in practice, but as more and more code is converted to use this helper, it's bound to happen eventually.
I'm not sure whether we should fix this by adding a reference count to the IDs, or just removing revoke() method entirely and relying on the finalization witness to do the cleanup. The latter has the drawback of keeping all mapping entries alive as long as the element that they point to continues to exist, which may become a problem if we create lots of short-lived references to lots of different elements, but I don't know how likely that is to happen in practice.
|
Assignee | |
Comment 1•5 years ago
|
||
UUIDs are expensive to create, to use as map keys, and to transfer between
processes. They are also completely unnecessary for this use case, since the
number returned by Math.random() has the same characteristics that we're
depending on the UUID generator for, only with better performance.
That said, even Math.random() is probably overkill for this use case, when we
can just use continually incremented serial number and get the same behavior,
but I decided to continue using randomized IDs mainly to minimize the
magnitued of the change.
|
|
Assignee | |
Comment 2•5 years ago
|
||
Requiring callers to manually cleanup all DOM references that they create runs
counter to the usual JavaScript model of automatic garbage collection, where
developers usually do not need to think about manual deallocation. It's bound
to lead to leaks before long.
This patch adds a finalization witness bound to the element of any DOM
reference that we create. When that element is destroyed, the finalization
witness also removes its entry from the ID map.
Since the mappings are already stored as weak references, this shouldn't
result in a change to the behavior that callers see, only in the underlying
memory management. It essentially makes this behave as a true weak value map.
|
|
Assignee | |
Comment 3•5 years ago
|
||
|
||
Comment 4•5 years ago
|
||
|
|
Assignee | |
Comment 5•5 years ago
|
||
|
||
Comment 6•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/e477d5f1f5db
https://hg.mozilla.org/mozilla-central/rev/ca6edf1bdf62
https://hg.mozilla.org/mozilla-central/rev/fbe06fda972e
|
||
Updated•5 years ago
|
Description
•