Closed Bug 638054 Opened 9 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jimb, Assigned: evilpie)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, sec-want, site-compat, Whiteboard: [adv-main58-])

Attachments

(4 files, 2 obsolete files)

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
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
See also bug 630370 for self-hosting Object.prototype.watch.
ES6 is going to define Object.observe.
Depends on: 800355
Assignee: general → nobody
Blocks: 1103158
Duplicate of this bug: 774645
Can we remove this in 58? The warning is in 57 (bug 934669).
Assignee: nobody → evilpies
Attached patch Remove the rest of watch/unwatch (obsolete) — Splinter Review
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)
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)
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)
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.
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.
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
Duplicate of this bug: 667129
Duplicate of this bug: 630370
Duplicate of this bug: 605687
Duplicate of this bug: 605596
Duplicate of this bug: 392072
Whiteboard: [adv-main58-]
You need to log in before you can comment on or make changes to this bug.