Closed Bug 1441168 Opened 3 years ago Closed 2 years ago
.promise Document Flushed should emit diagnostic information when read-only treatment of the DOM is violated
47 bytes, text/x-phabricator-request
|Details | Review|
Bug 1441168 - Add some documentation to Window.webidl about DOM modifications within promiseDocumentFlushed callbacks. r?ehsan
47 bytes, text/x-phabricator-request
|Details | Review|
In bug 1434376, I added a new ChromeOnly function, promiseDocumentFlushed, to Window.webidl. Here's the documentation for the function: https://hg.mozilla.org/mozilla-central/file/0961e7b6ec48/dom/webidl/Window.webidl#l449 One of the things I wanted to do, but ultimately had to punt on due to time, was make it impossible for consumers of this API to violate the constraint that style and layout information should only be read within the promiseDocumentFlushed callback, and that attempts to _write_ to the DOM (which might queue up a style or layout flush) are forbidden. Right now, nothing stops anybody from dirtying the DOM inside a promiseDocumentFlushed callback. I'd like to make it impossible, to ensure that the promiseDocumentFlushed callbacks are a reliable way to query for style and layout information without ever flushing. Ideas: 1) Introduce some kind of primitive onto the stack that makes DOM write operations throw an exception inside the JS callback, and then place this on the stack before calling the callback. I don't think anything like this exists, so we'd have to invent it. The exception would then cause the Promise to be rejected. 2) After the callback is called, check to ensure that now style or layout flushes have been queued, and if so, log to the console and reject the Promise. This is a little less rigorous since it _allows_ the DOM write to actually occur. I'd actually like to pursue (1) if possible, simply because it'd be more likely that this would get a front-end developers attention if they violated the constraint.
I would also like (1) since it would give an exact stack trace of where the write occurred, instead of having to guess.
Note that (2) also has the characteristic that if one were to dirty the DOM, and then cause a flush from within the callback (the ultimate violation!) the check for needed style flushes or reflows would come back negative. So that's another reason why we should pursue (1). If something more like (2) ends up being what we have to do, then we need to address the above case.
Is it possible to prioritize this? I was just trying to write a patch for bug 1483354 and I noticed that we're basically doing this already: https://searchfox.org/mozilla-central/rev/2edebf41a2b2e624859bf15ed5a98bdd002f73b4/browser/base/content/urlbarBindings.xml#1382 https://searchfox.org/mozilla-central/rev/2edebf41a2b2e624859bf15ed5a98bdd002f73b4/browser/components/urlbar/UrlbarInput.jsm#383 (same as previous but different place) (bug 1511805) I also noticed bug 1511804 ... so I wonder if, at a minimum, we could add an eslint rule that enforces that people (1) call the function and (2) use the result value. Hopefully that'd catch some of these...
My try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c72ddc5566eda2965f750e120cbf91095e7ddf0b&selectedJob=216261768 is happily exploding because of bug 1511805. What might be simplest is to fix bug 1511805, and then try to land this, to "lock the horse back in the barn", so to speak.
I have a patch up for bug 1511805. Here's a try push with both that and this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51434504341c8f71e969c45b1a4cd98871ccb3f1
Yay, looking pretty green! Will request review.
Attachment #9030325 - Attachment description: Bug 1441168 - Make promiseDocumentFlushed reject if the DOM is modified in the callback. → Bug 1441168 - Make promiseDocumentFlushed reject if the DOM is modified in the callback. r?Gijs
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/185b7717cc2e Make promiseDocumentFlushed reject if the DOM is modified in the callback. r=Gijs
Backout by email@example.com: https://hg.mozilla.org/integration/autoland/rev/d8e076214d14 Backed out changeset 185b7717cc2e for causing devtools leak failures on browser_webconsole_split.js CLOSED TREE
Backed out changeset 185b7717cc2e (bug 1441168) for causing devtools leak failures on browser_webconsole_split.js CLOSED TREE Backout revision https://hg.mozilla.org/integration/autoland/rev/185b7717cc2ec25a4d1e6c7fea86a63838479c1a Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=218139602&revision=185b7717cc2ec25a4d1e6c7fea86a63838479c1a Log failure https://treeherder.mozilla.org/logviewer.html#?job_id=218140952&repo=autoland https://treeherder.mozilla.org/logviewer.html#?job_id=218141788&repo=autoland https://treeherder.mozilla.org/logviewer.html#?job_id=218141784&repo=autoland :mconley Please take a look
Depends on: 1516022
Depends on D14101
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/aa12998cd7e2 Make promiseDocumentFlushed reject if the DOM is modified in the callback. r=Gijs https://hg.mozilla.org/integration/autoland/rev/b2c9ad0fa546 Add some documentation to Window.webidl about DOM modifications within promiseDocumentFlushed callbacks. r=Ehsan
You need to log in before you can comment on or make changes to this bug.