Page MenuHomePhabricator

visitDOM makes an invalid assumption about its handlers
Closed, InvalidPublic

Description

visitDOM assumes that handlers are static functions and won't references a 'this' because of handler.apply(null, args). However, this is not always true. Specifically, consider the commonly used visitDOM(... storeDataAttribs). and storeDataAttribs in turn calls 'storeInPageBundle'. This doesn't break right now because many of those use the "DU." pattern with a reference to a global object. However, when I refactored DOMUtils and moved it to ContentUtils, I changed "DU." references to "this" where appropriate, and now jenkins complains about it in https://integration.wikimedia.org/ci/job/parsoidsvc-npm-run-roundtrip-node-6-docker/2021/console

Ideally, visitDOM wouldn't assume that the handler's object is null.

Event Timeline

ssastry triaged this task as Medium priority.Nov 17 2018, 7:34 PM
ssastry moved this task from Needs Triage to Tech Debt / Big changes on the Parsoid board.

Change 480266 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/services/parsoid@master] Use spread operator instead of apply in visitDOM

https://gerrit.wikimedia.org/r/480266

For this helper to work the way you want, you'd either need to bind the handler before passing it in or pass in a context to apply to. The execution context of a reassigned method just isn't what this task implies it should be.

For this helper to work the way you want, you'd either need to bind the handler before passing it in or pass in a context to apply to. The execution context of a reassigned method just isn't what this task implies it should be.

See my comment on the patch.

Change 480266 merged by jenkins-bot:
[mediawiki/services/parsoid@master] Add helpers to ease binding context when load/storing data attribs

https://gerrit.wikimedia.org/r/480266