| Summary: | REGRESSION (r125251): wrapper lifetimes of SVGElementInstance are incorrect; assertion seen in svg/custom/use-instanceRoot-event-listeners.xhtml | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
| Component: | WebCore JavaScript | Assignee: | Darin Adler <darin> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, cgarcia, commit-queue, darin, dino, eric.carlson, glenn, jer.noble, kondapallykalyan, ossy, philipj, roger_fong, sergio, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Mark Lam
2014-04-24 15:16:11 PDT
Created attachment 230145 [details]
Patch
Comment on attachment 230145 [details]
Patch
r=me. I'm so happy you did this!
You'll probably need to run "run-bindings-tests --reset-results" before landing this.
(In reply to comment #4) > You'll probably need to run "run-bindings-tests --reset-results" before landing this. I did. Surprisingly, none of the bindings tests cover JSCustomMarkFunction. Committed r167794: <http://trac.webkit.org/changeset/167794> *** Bug 93812 has been marked as a duplicate of this bug. *** Mark says this bug still happens; apparently I was testing without JSC_slowPathAllocsBetweenGCs. When I tried to do some logging by also setting JSC_logGC to 2, I started getting a different assertion, ASSERT(m_opaqueRoots.isEmpty()) failed in SlotVisitor::didStartMarking at the start of one collection. OK, I figured out what the second problem is. The bindings for addEventListener and removeEventListener assume that the EventTarget object itself is the object which has a wrapper that keeps alive the listener’s wrapper. But for an SVGElementInstance, the instance itself simply attaches the listener to the target, therefore the SVGElementInstance object doesn’t keep the listener’s wrapper alive, its target does. We have to fix the addEventListener binding. There’s event code in CodeGeneratorJS.pm that knows how to generate this code correctly. However, two years ago, <http://trac.webkit.org/changeset/125251> broke this by removing the code to generate the add/removeEventListener function. I can see two ways to fix this. The obvious way to do it would be to re-add the addEventListener/removeEventListener functions to SVGElementInstance somehow. But that would be dangerous since there is nothing that would prevent someone from calling the base EventTarget addEventListener/removeEventListener function and passing it an SVGElementInstance. I guess no fix like this can ever be entirely complete. I suppose the better way to fix this would be to change all addEventListener function calls to call a virtual function, which in the case of SVGElementInstance would return the correspondingElement. Created attachment 244861 [details]
Patch
*** Bug 93812 has been marked as a duplicate of this bug. *** Comment on attachment 244861 [details] Patch Clearing flags on attachment: 244861 Committed r178633: <http://trac.webkit.org/changeset/178633> All reviewed patches have been landed. Closing bug. This broke bindings generation tests, <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/2072/steps/bindings-generation-tests/logs/stdio>. All the changes seems expected, so I'll re-generate results and land them. |