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

RESOLVED FIXED in Firefox 67

Status

()

RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Depends on: 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(2 attachments)

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

a year 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

a year 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

a year ago
See Also: → bug 1442218

Updated

4 months ago
Blocks: 1511805

Comment 3

4 months 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)

Updated

4 months ago
Assignee: nobody → mconley
(Assignee)

Updated

4 months ago
Flags: needinfo?(mconley)
(Assignee)

Comment 5

4 months 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.
No longer blocks: 1511805
Depends on: 1511805
(Assignee)

Comment 6

3 months 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

3 months ago
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

Comment 8

3 months ago
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

Comment 9

3 months ago
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
(Assignee)

Updated

2 months ago
Depends on: 1516022
Flags: needinfo?(mconley)
(Assignee)

Updated

2 months ago
Depends on: 1519407

Comment 12

2 months 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

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months 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.