Jeff Walden [:Waldo] Reporter
	Description • 11 years ago

To get rid of these, we need to tell developers not to use them.  This patch does so, warning once per global when these functions are used.

	Kohei Yoshino [:kohei]
	Comment 1 • 11 years ago

I'll add this bug to a site compatibility document on MDN to bring developers' attention, once the patch is landed.

	Masatoshi Kimura [:emk]
	Comment 2 • 11 years ago

Removing without any replacement?

	Jeff Walden [:Waldo] Reporter
	Comment 3 • 11 years ago

The replacement is to define the relevant property as a getter/setter pair to observe changes to it.  (Note that watch/unwatch are already mostly broken on such properties, because they can't observe the initial value of the property, because they obviously can't invoke the getter to determine it.)

Object.observe is a separate thing, separate functionality, and it doesn't provide the same power that a getter/setter pair provides (specifically, the ability to intercept and change the value-to-be-set.)  It certainly doesn't block making this warn.  And given its lesser power compared to the getter/setter interception solution, it's not even an adequate replacement for watch/unwatch anyway.

	Tom S [:evilpie] Assignee
	Comment 4 • 11 years ago

Comment on attachment 826994 [details] [diff] [review]
Patch

I want to have this, thus stealing.

	Tom S [:evilpie] Assignee
	Comment 5 • 11 years ago

Comment on attachment 826994 [details] [diff] [review]
Patch

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

::: js/src/js.msg
@@ +244,4 @@
>  MSG_DEF(JSMSG_UNUSED191,              191, 0, JSEXN_NONE, "")
>  MSG_DEF(JSMSG_UNUSED192,              192, 0, JSEXN_NONE, "")
>  MSG_DEF(JSMSG_BAD_FOR_EACH_LOOP,      193, 0, JSEXN_SYNTAXERR, "invalid for each loop")
> +MSG_DEF(JSMSG_OBJECT_WATCH_DEPRECATED,194, 0, JSEXN_TYPEERR, "Object.prototype.watch and unwatch are very slow, non-standard, and deprecated; use a getter/setter instead")

maybe Proxy?

::: js/src/vm/GlobalObject.cpp
@@ +515,5 @@
> +                                          JSMSG_OBJECT_WATCH_DEPRECATED))
> +        {
> +            return false;
> +        }
> +        v.init(global, HeapSlot::Slot, WARNED_WATCH_DEPRECATED, BooleanValue(true));

I am not sure about this, isRuntimeCodeGenEnabled uses .set, but .init makes more senes. The previous value is undefined so we shouldn't need the per-barrier.

::: js/src/vm/GlobalObject.h
@@ +119,5 @@
> +     * we won't expose GlobalObject, so just assert that the two values are
> +     * synchronized.
> +     */
> +    static_assert(JSCLASS_GLOBAL_SLOT_COUNT == RESERVED_SLOTS,
> +                  "global object slot counts are inconsistent");

:)

@@ +559,5 @@
>      static bool isRuntimeCodeGenEnabled(JSContext *cx, Handle<GlobalObject*> global);
>  
> +    // Warn about use of the deprecated watch/unwatch functions in the global
> +    // in which |obj| was created, if no prior warning was given.
> +    static bool warnAboutWatchDeprecation(JSContext *cx, HandleObject obj);

DOM uses the function WarnOnceAbout, so maybe like WarnOnceAboutWatch?

	Jason Orendorff [:jorendorff]
	Comment 6 • 11 years ago

This deprecation warning is fine to go in, in case there was any question.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 7 • 11 years ago

Firebug is using watch/unwatch to allow the user to break into a debugger when observed property changes its value (the user can set a breakpoint on the property) - this helps to find the specific line of code that causes the modification.

What alternative API should we used instead if this one is deprecated?

Honza

	Jason Orendorff [:jorendorff]
	Comment 8 • 11 years ago

Hmm. Firebug is exceptional. Object.observe supercedes most Object.prototype.watch use cases, accessors can be used for others, but Firebug is a debugger and needs a real Debugger watchpoint feature.

There is no replacement yet.

Jeff, disagree with me.

	Jason Orendorff [:jorendorff]
	Comment 9 • 11 years ago

Comment on attachment 826994 [details] [diff] [review]
Patch

Actually we should probably have this discussion before the warning goes in. Let me put a "hold" on this. (sr-)

Sorry for the mixed signals. Actual ambivalence.

	Jeff Walden [:Waldo] Reporter
	Comment 10 • 11 years ago

Welp, I pushed this to try this morning (early), then when I showed up now and saw that green, I pushed without checking here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/877d4860a9f2

So, um.  Discussion to commence on IRC, or something.

	Jeff Walden [:Waldo] Reporter
	Comment 11 • 11 years ago

This got backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f96acad66038 following that discussion.  I'll likely reland the change, with the two method calls that trigger warnings commented out, sometime in the next day or so.

Dealing with the Firebug issue, it sounds like, requires either a total hackaround (friend method, expose as Components.utils.deprecatedWatch or similar), or implementing a lightweight version of the setPropertyWatchpoint/clearPropertyWatchpoint methods from <https://wiki.mozilla.org/Debugger>.  jorendorff thinks it's about equal effort to do either way.  Given I haven't done debugger work, I may try that tack, to learn something about that area of code.  But we'll see -- I need to debug bug 903332's patch (which breaks *only* on b2g-desktop, sigh) first.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 12 • 11 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)

> or implementing a lightweight version of the
> setPropertyWatchpoint/clearPropertyWatchpoint methods from
> <https://wiki.mozilla.org/Debugger>.
I like this ideas and note that other dev-tools (extensions or built-in)
could utilize this API soon as well.


Honza

	Jeff Walden [:Waldo] Reporter
	Comment 13 • 11 years ago

Relanded, with the actual calls to perform the warn-once behavior commented out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa75e9c102a

Still need to do the other stuff, but at least this means I'm not carrying around a larger patch for a longer time.

	Kohei Yoshino [:kohei]
	Comment 14 • 11 years ago

How about making Object.watch/unwatch chrome-only?

	Jeff Walden [:Waldo] Reporter
	Comment 15 • 11 years ago

The JS engine has no concept of chrome/non-chrome at all on which to rely to expose/not-expose this.  And script versions are a property of *scripts*, not of global objects, so that's not a reasonable way to distinguish either.

	Carsten Book [:Tomcat]
	Comment 16 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1fa75e9c102a

	Kohei Yoshino [:kohei]
	Comment 17 • 9 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/object-prototype-watch-will-be-removed/

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 9 years ago

> The JS engine has no concept of chrome/non-chrome at all

How about we just fix that?  Or just add an "expose Object.prototype.watch" thing on CompartmentOptions and let the browser set it as needed?

	Jan de Mooij [:jandem]
	Comment 19 • 9 years ago

There's compartment->isSystem (and compartment->addonId). Not sure if that's set for all compartments we're interested in though.

	Jeff Walden [:Waldo] Reporter
	Comment 20 • 8 years ago

(In reply to Boris Zbarsky [:bz] from comment #18)
> Or just add an "expose Object.prototype.watch"
> thing on CompartmentOptions and let the browser set it as needed?

Hmm, yeah, we could do this.  Can you point to the places that create web-visible global objects, so I can know which places would need option-toggling?  (Or their complement.)

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 8 years ago

Per IRC discussion, you actually want the non-web-visible globals, because you want to default these things to off.

Those would be:

1)  Various worker globals, if for chrome.  See the various WrapGlobalObject functions in dom/workers/WorkerScope.cpp.

2)  CreateNativeGlobalForInner in dom/base/nsGlobalWindow.cpp if it's a chrome window.

3)  Anything passing through nsXPConnect::InitClassesWithNewWrappedGlobal (this covers message managers,
    sandboxes, etc).

	Jeff Walden [:Waldo] Reporter
	Comment 22 • 8 years ago

This is probably pretty close to a fix.  No testing done to see whether any existing tests need changes -- almost certainly a bunch of watch-tests need changes to mark them shell-only, but that's all I'm definitely aware of.

	Jan de Mooij [:jandem]
	Comment 23 • 8 years ago

Could we include uneval/toSource et al, maybe even legacy generators? Like a |bool exposeLegacyFeatures| or something...

	Tom S [:evilpie] Assignee
	Comment 24 • 8 years ago

I think removing watch/unwatch from the Spec array will break JS Xrays.

	Tom S [:evilpie] Assignee
	Comment 25 • 8 years ago

I think Firebug 2.0 is supposed to be deprecated, because it doesn't work with e10s (https://blog.getfirebug.com/2016/06/07/unifying-firebug-firefox-devtools/). This would be a good time to start showing a warning about Object.prototype.watch.

	Tom S [:evilpie] Assignee
	Comment 26 • 7 years ago

Now that Firebug is dead, we should be able to warn about this. In Hawaii I talked to someone working on devtools and they were interested in doing something like bug 638053 to add a debugger API, but I haven't heard anything new about that.

	Jan de Mooij [:jandem]
	Comment 27 • 7 years ago

Watchpoints are buggy because the JITs don't check for them everywhere, and they slow down property sets. I suppose adding the warning ASAP is better than nothing, but not sure it's required before removing this 'feature'.

	Tom S [:evilpie] Assignee
	Comment 28 • 7 years ago

All the new warnOnce flags are on the JSCompartment, so let's remove this last unused one from the GlobalObject.

	Tooru Fujisawa [:arai]
	Comment 29 • 7 years ago

Comment on attachment 8895782 [details] [diff] [review]
Remove the old GlobalObject warnOnce code

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

nice cleanup :D

	Tooru Fujisawa [:arai]
	Comment 31 • 7 years ago

Comment on attachment 8898950 [details] [diff] [review]
Warn about watch/unwatch

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

r+ with the comment addressed.

::: js/src/jscompartment.h
@@ +624,5 @@
>      bool                         warnedAboutDateToLocaleFormat : 1;
>      bool                         warnedAboutExprClosure : 1;
>      bool                         warnedAboutForEach : 1;
>      bool                         warnedAboutLegacyGenerator : 1;
> +    bool                         warnedAboutObjectWatch : 1;

this needs initialization in ctor, next to the following line:

https://dxr.mozilla.org/mozilla-central/rev/a6a1f5c1d971dbee67ba6eec7ead7902351ddca2/js/src/jscompartment.cpp#62

::: js/src/tests/js1_2/Objects/watch-deprecated.js
@@ +10,5 @@
> +assertEq(warning.message.includes("watch"), true, "warning should mention watch");
> +
> +clearLastWarning();
> +
> +var g = newGlobal();

`var` here can be omitted

@@ +13,5 @@
> +
> +var g = newGlobal();
> +g.eval("({}).unwatch('x')");
> +
> +var warning = getLastWarning();

here too

	Pulsebot
	Comment 32 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/093d449b0d56
Remove the old GlobalObject warnOnce code. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/10d25965b83a
Warn about watch/unwatch. r=arai

	Phil Ringnalda (:philor)
	Comment 33 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/093d449b0d56
https://hg.mozilla.org/mozilla-central/rev/10d25965b83a

	Kohei Yoshino [:kohei]
	Comment 34 • 7 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/object-prototype-watch-will-be-removed/

	Florian Scholz (Open Web Docs)
	Comment 35 • 7 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/57#JavaScript
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