Closed Bug 934669 Opened 10 years ago Closed 6 years ago

Deprecate Object.prototype.{,un}watch, and make them warn when used


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: evilpie)



(Keywords: dev-doc-complete, site-compat)


(3 files, 1 obsolete file)

Attached patch PatchSplinter Review
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.
Attachment #826994 - Flags: review?(jorendorff)
I'll add this bug to a site compatibility document on MDN to bring developers' attention, once the patch is landed.
Removing without any replacement?
Depends on: 800355
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.
No longer depends on: 800355
Attachment #826994 - Flags: review?(jorendorff)
Comment on attachment 826994 [details] [diff] [review]

I want to have this, thus stealing.
Attachment #826994 - Flags: review?(evilpies)
Comment on attachment 826994 [details] [diff] [review]

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, " 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.
> +     */
> +                  "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?
Attachment #826994 - Flags: review?(evilpies) → review+
This deprecation warning is fine to go in, in case there was any question.
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?

Hmm. Firebug is exceptional. Object.observe supercedes most 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.
Comment on attachment 826994 [details] [diff] [review]

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.
Attachment #826994 - Flags: superreview-
Welp, I pushed this to try this morning (early), then when I showed up now and saw that green, I pushed without checking here.

So, um.  Discussion to commence on IRC, or something.
This got backed out in 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 <>.  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.
Whiteboard: [leave open]
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)

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

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

Still need to do the other stuff, but at least this means I'm not carrying around a larger patch for a longer time.
How about making chrome-only?
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.
Depends on: 800355
No longer depends on: 800355
Blocks: 1103158
> The JS engine has no concept of chrome/non-chrome at all

How about we just fix that?  Or just add an "expose" thing on CompartmentOptions and let the browser set it as needed?
There's compartment->isSystem (and compartment->addonId). Not sure if that's set for all compartments we're interested in though.
Blocks: 638054
(In reply to Boris Zbarsky [:bz] from comment #18)
> Or just add an "expose"
> 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.)
Flags: needinfo?(bzbarsky)
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).
Flags: needinfo?(bzbarsky)
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.
Could we include uneval/toSource et al, maybe even legacy generators? Like a |bool exposeLegacyFeatures| or something...
I think removing watch/unwatch from the Spec array will break JS Xrays.
I think Firebug 2.0 is supposed to be deprecated, because it doesn't work with e10s ( This would be a good time to start showing a warning about
No longer blocks: 1103158
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.
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'.
Flags: needinfo?(shu)
Assignee: jwalden+bmo → evilpies
All the new warnOnce flags are on the JSCompartment, so let's remove this last unused one from the GlobalObject.
Attachment #8895782 - Flags: review?(arai.unmht)
Comment on attachment 8895782 [details] [diff] [review]
Remove the old GlobalObject warnOnce code

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

nice cleanup :D
Attachment #8895782 - Flags: review?(arai.unmht) → review+
Attachment #8898950 - Flags: review?(arai.unmht)
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:

::: 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
Attachment #8898950 - Flags: review?(arai.unmht) → review+
Attachment #8717235 - Attachment is obsolete: true
Pushed by
Remove the old GlobalObject warnOnce code. r=arai
Warn about watch/unwatch. r=arai
Closed: 6 years ago
Flags: needinfo?(shu)
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.