1441168 - window.promiseDocumentFlushed should emit diagnostic information when read-only treatment of the DOM is violated

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Mike Conley (:mconley) (:⚙️)]

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

[:Paolo Amadini]

I would also like (1) since it would give an exact stack trace of where the write occurred, instead of having to guess.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[:Gijs (he/him)]

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...

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1441168 - Make promiseDocumentFlushed reject if the DOM is modified in the callback. r?Gijs

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

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

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Yay, looking pretty green! Will request review.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/185b7717cc2e
Make promiseDocumentFlushed reject if the DOM is modified in the callback. r=Gijs

Comment 9

[Pulsebot]

Backout by [e-mail]:
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

[Arthur Iakab [arthur_iakab]]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1441168 - Add some documentation to Window.webidl about DOM modifications within promiseDocumentFlushed callbacks. r?ehsan

Depends on D14101

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/aa12998cd7e2
https://hg.mozilla.org/mozilla-central/rev/b2c9ad0fa546