Closed Bug 1441168 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: General, enhancement)

enhancement
Not set
normal

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.
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.
See Also: → 1442218
Blocks: 1511805
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: nobody → mconley
Flags: needinfo?(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.
No longer blocks: 1511805
Depends on: 1511805
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 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
Depends on: 1516022
Flags: needinfo?(mconley)
Depends on: 1519407
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.