JavaScript Object.prototype.watch should be removed, once an adequate debugger-only replacement exists

RESOLVED FIXED in Firefox 58

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
a month ago

People

(Reporter: jimb, Assigned: evilpie)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {dev-doc-complete, sec-want, site-compat})

unspecified
mozilla58
dev-doc-complete, sec-want, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
The 'Object.prototype.watch' function has wonky, incomplete semantics, is non-standard, and serves as a ready source of serious bugs because it is available to content.

The Debug object (bug 636907) should provide watchpoint functionality instead. Once it is good enough, Object.prototype.watch should be deleted.
Blocks: 774645

Comment 1

5 years ago
The debug object probably won't be available to web content (see bug 636907 comment 10).  So we might as well disable Object.prototype.watch *for web content* right now.

Web content can use standard techniques such as Object.defineProperty for most non-"debugger" uses of watch.

What debuggers currently use Object.prototype.watch?
Keywords: sec-want

Comment 2

5 years ago
See also bug 630370 for self-hosting Object.prototype.watch.
ES6 is going to define Object.observe.

Comment 4

5 years ago
http://weblog.bocoup.com/javascript-object-observe/

Updated

5 years ago
Depends on: 800355
Assignee: general → nobody

Updated

2 years ago
Blocks: 1103158

Updated

2 years ago
Depends on: 934669

Updated

2 years ago
Keywords: dev-doc-needed, site-compat
Duplicate of this bug: 774645
Can we remove this in 58? The warning is in 57 (bug 934669).
(Assignee)

Updated

2 months ago
Assignee: nobody → evilpies
(Assignee)

Comment 7

2 months ago
Created attachment 8916700 [details] [diff] [review]
Remove watch class-hook and proxy trap
(Assignee)

Comment 8

2 months ago
Created attachment 8916701 [details] [diff] [review]
Remove the rest of watch/unwatch
(Assignee)

Comment 9

2 months ago
Created attachment 8920577 [details] [diff] [review]
v1 - Remove watch class-hook and proxy trap

I put this in a different patch, because only this part really touches code outside js/ and the rest is relatively self-contained.
Attachment #8916700 - Attachment is obsolete: true
Attachment #8920577 - Flags: review?(jorendorff)
Attachment #8920577 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

2 months ago
Created attachment 8920578 [details] [diff] [review]
v1 - Remove the guts of the watch/unwatch implementation

I was almost certain I would have to touch a lot more places, but unless I missed something obvious the interactions with the JITs etc are actually quite limited.
Attachment #8916701 - Attachment is obsolete: true
Attachment #8920578 - Flags: review?(jorendorff)
(Assignee)

Comment 11

2 months ago
Created attachment 8920580 [details] [diff] [review]
Mostly remove JS tests using watch/unwatch

I kind of followed these steps before deleting some tests:
1) Test name or summary mentions watch/unwatch -> delete
2) Test is very short and watch is a major part -> delete (We had like 50 tests just for __defineGetter__ and watch ...)
3) Complex test using watch -> tried to keep it working

In general I mostly made my life easier by aggressively removing fuzz tests using watch instead of trying to somehow fix them by hand.
Attachment #8920580 - Flags: review?(jorendorff)
(Assignee)

Comment 12

2 months ago
I am still working on the patch to remove watch/unwatch from DOM and other tests, mostly because I don't want to run all of these locally.
(Assignee)

Comment 13

2 months ago
Created attachment 8920674 [details] [diff] [review]
v1 - Remove or fix tests outside JS using watch/unwatch

Nick please review the devtools test. Boris, I found the codegen.py hunk while fixing the tests, I could move it to the first path.
Attachment #8920674 - Flags: review?(nfitzgerald)
Attachment #8920674 - Flags: review?(bzbarsky)
Comment on attachment 8920577 [details] [diff] [review]
v1 - Remove watch class-hook and proxy trap

r=me on the parts outside js/
Attachment #8920577 - Flags: review?(bzbarsky) → review+
Comment on attachment 8920674 [details] [diff] [review]
v1 - Remove or fix tests outside JS using watch/unwatch

r=me on the dom/bindings and js/xpconnect changes.
Attachment #8920674 - Flags: review?(bzbarsky) → review+
Comment on attachment 8920674 [details] [diff] [review]
v1 - Remove or fix tests outside JS using watch/unwatch

Review of attachment 8920674 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8920674 - Flags: review?(nfitzgerald) → review+
Comment on attachment 8920577 [details] [diff] [review]
v1 - Remove watch class-hook and proxy trap

Review of attachment 8920577 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/EnvironmentObject.cpp
@@ +1,1 @@
> +

Accidental blank line.
Attachment #8920577 - Flags: review?(jorendorff) → review+
Attachment #8920578 - Flags: review?(jorendorff) → review+
Comment on attachment 8920580 [details] [diff] [review]
Mostly remove JS tests using watch/unwatch

Review of attachment 8920580 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you.
Attachment #8920580 - Flags: review?(jorendorff) → review+
Glad to see the end of this!

I mentioned in #devtools on IRC that we were removing this stuff. I said that doing this now does not make it much harder to add watchpoints to the Debugger later, if the devtools team ever gets around to that. At worst, we could just add this stuff back. At best, we would want something even less intrusive. Either way, most of the work would be testing, updating the Debugger protocol, etc. And it seems to me most of the risk is getting GC right.

The testing patch reinforces this. Most of the tests we're deleting are fuzztests from earlier days, when watchpoints were implemented using JSPropertyOps---which was faster, but intrusive and prone to bugs.

Comment 20

2 months ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8f9e5f2858
Remove watch class-hook and proxy trap r=jorendorff,bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f864ad5779
Remove the guts of the watch/unwatch implementation. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/31bc65f1acab
Mostly remove JS tests using watch/unwatch. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/a124f4901430
Remove or fix tests outside JS using watch/unwatch. r=bz,fitzgen
Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/object-prototype-watch-has-been-removed/
https://hg.mozilla.org/mozilla-central/rev/3f8f9e5f2858
https://hg.mozilla.org/mozilla-central/rev/e4f864ad5779
https://hg.mozilla.org/mozilla-central/rev/31bc65f1acab
https://hg.mozilla.org/mozilla-central/rev/a124f4901430
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/58#JavaScript

Reference pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/watch
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/unwatch

Compat data:
https://github.com/mdn/browser-compat-data/pull/590
Keywords: dev-doc-needed → dev-doc-complete

Updated

a month ago
Duplicate of this bug: 667129

Updated

a month ago
Duplicate of this bug: 630370

Updated

a month ago
Duplicate of this bug: 605687

Updated

a month ago
Duplicate of this bug: 605596

Updated

a month ago
Duplicate of this bug: 392072
You need to log in before you can comment on or make changes to this bug.