Closed
Bug 1441168
Opened 7 years ago
Closed 6 years ago
window.promiseDocumentFlushed should emit diagnostic information when read-only treatment of the DOM is violated
Categories
(Core :: General, enhancement)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla67
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: mconley, Assigned: mconley)
References
(Depends on 1 open bug)
Details
Attachments
(2 files)
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.
Comment 1•7 years ago
|
||
I would also like (1) since it would give an exact stack trace of where the write occurred, instead of having to guess.
Assignee | ||
Comment 2•7 years ago
|
||
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.
Updated•7 years ago
|
Comment 3•6 years ago
|
||
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...
Flags: needinfo?(mconley)
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mconley
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(mconley)
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
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
Assignee | ||
Comment 7•6 years ago
|
||
Yay, looking pretty green! Will request review.
Updated•6 years ago
|
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 mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/185b7717cc2e
Make promiseDocumentFlushed reject if the DOM is modified in the callback. r=Gijs
Backout by aiakab@mozilla.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
Comment 10•6 years ago
|
||
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
Flags: needinfo?(mconley)
Assignee | ||
Comment 11•6 years ago
|
||
Depends on D14101
Comment 12•6 years ago
|
||
Pushed by mconley@mozilla.com:
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
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aa12998cd7e2
https://hg.mozilla.org/mozilla-central/rev/b2c9ad0fa546
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox67:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in
before you can comment on or make changes to this bug.
Description
•