User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release) Build ID: 20130730113002 Steps to reproduce: Attempting to access Citrix webaccess system after Firefox updated to V23 Actual results: Applications list failed to open. When checking the Javascript error screen, a message is repeated every second "TypeError: can't watch non-native objects of class Proxy" This did not occur in previous versions. The relevant code (from the file I think supplied by Citrix) is: if (document.watch) { document.watch('cookie',function (id,ov,nv) { var $r = new XMLHttpRequest(); $r.open("POST","/cvpn/cp",false); $r.send(nv); return nv; }); Expected results: Application list should have appeared, as per previous FF versions.

Is it possible to have kind of testcase, maybe online access to Citrix webaccess system (with guest account and limited rights e.g.) to test and debug? If not, install the tool mozregression (Citrix webaccess system) and find a regression range. Firefox 23 builds started in April 2013: mozregression --good=2013-04-01 Then post the output of mozreg.

Is that code doing anything weird with Proxy objects? Otherwise, sure sounds like a change in JS behavior, so tentatively moving to Core::JavaScript Engine.

I'd lay good odds on this having changed because we switched document over to WebIDL, converting it from a non-proxy object to a proxy. That sensibly fixed all sorts of issues, but it happened to break (the thoroughly non-standard, and never-gonna-be-standard) Object.prototype.watch method. Hopefully one of the CCs can confirm that hypothesis...

HTMLDocument instances are proxies, yes, because they have a nemed getter. That said, document.watch('cookie') was actually somewhat meaningful, kinda: that's only ever changed from script, as long as no one add a <div id="cookie"> to the document. Given that this is breaking actual sites, it would be good to try fixing this. In this particular case, we could fake something with basically interposing a setter that triggers the passed-in callback function. Or we could make the watch() call silently no-op and hope the caller doesn't actually care.... Jeff, thoughts?

Let's do nothing when setting a watch on an object that can't take them.

Sorry there is no way to get you access - it's a government site, requires keycodes that change every 30 seconds etc - but am happy to carry out tests if you let me know exactly. what is needed. Had a look at the last link from Jeff Walden - was still building, so nothing for me to grab/look at. Bug also affects Linux (Debian) build as well. Have install mozregression on the linux box and will commence testing.

Last good nightly: 2013-05-05 First bad nightly: 2013-05-06 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c8e47b184aba&tochange=b109e2dbf03b

Given your comments above, noted this in the push log a2245b038bcc Peter Van der Beken — Bug 855971 - Switch HTMLDocument to WebIDL bindings. r=bz.

Right. That's when document became a proxy.

With the build (Win32 one tested, as it had completed) with the modification you made, it now goes into an infinite loop reloading the auth/login page from the Citrix system, so that didn't resolve the issue. Looking at the Citrix JS code, once document.watch returns true, it assumes it will work, so now returning false instead of an error on the attempted assignment does't stop them assuming that it will succeed and send the cookie whenever it changes. Because now the cookie never gets sent, it appears to go into an auth loop (possibly requesting the auth token? It was hard to tell not knowing their actual code). So - this still breaks Citrix Webaccess, but it looks like they have a problem with their Javascript assumptions too (assuming placing the Watch will always succeed).

The good news of HTMLDocument having moved to WebIDL is that... it's WebIDL compliant, so it's possible to hack on it. The following piece of code allows to watch changes to document.cookies in a standard way. https://gist.github.com/DavidBruant/6199886 @Vark: I made the code under CC0 to avoid any legal issues, so either you or Citrix (if you have a way to contact them) can use it at will. When all browsers will have moved to WebIDL, this should work everywhere. I have only tested in Firefox 24, but it may work in other browsers (maybe even Firefox 23). I'll let you do the testing in browsers you need to support. It's not necessarily a recommended practice to modify built-in setters, but it's still better than using Object.watch which is expected to disappear eventually (hopefully...), replaced by the upcoming standard Object.observe. Ideally, the code snippet I linked to would be preceded by a feature-test to test whether this will work in the current browser, but I'm not sure how to write that. Maybe something like: // insert code snippet that modifies HTMLDocument.prototype cookie setter // (maybe wrapped in a try/catch block for browsers where that'd fail) // at least, detect failure and don't set setCookieWatcher if it doesn't work as expected if(typeof setCookieWatcher === 'function'){ setCookieWatcher(watcher) } else{ if(document.watch){ try{ document.watch('cookie', watcher); } catch(e){ // no watching or other solution } } else{ // no watching or other solution } }

Comment on attachment 788375 [details] [diff] [review] Do nothing when attempting to set a watch on a non-native object Review of attachment 788375 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdbgapi.cpp @@ -322,4 @@ > return false; > > if (!obj->isNative()) { > - JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_CANT_WATCH, I think this makes JSMSG_CANT_WATCH unused.

@David - Thanks for the code, unfortunately it's of no use to me because I can't place it on the site. I can pass that back to the tech support for the health department that runs this particular instance of Webaccess, but I don't know if it's within code they can modify (as this is a standard Citrix supplied product - not sure of their licencing in regards to making changes, and the department change process is long and arduous!) @All - To me, as an end user (albeit one that also does web development), this is still a breakage of a site that did work, and does still work under other browsers, and is a site built by a major company (Citrix) so would, to me, be expected to continue working. The fact that it's a non-standard feature and, as you say, planned to disappear, is something I'll also ask the tech dept to pass back to Citrix. It would be nice if there was some way to tell Firefox to return "False" for document.watch, because then the site would use it's fallback method - or is there any way to do that? Anyone know if/when it's planned to be obsoleted and/or removed, because that might be useful to pass on too.

(In reply to Vark from comment #14) > @David - Thanks for the code, unfortunately it's of no use to me I imagined, but that was in case. It might also help people coming across the same problem than you and finding this bug :-) > @All - To me, as an end user (albeit one that also does web development), > this is still a breakage of a site that did work, and does still work under > other browsers, and is a site built by a major company (Citrix) so would, to > me, be expected to continue working. Mozilla takes websites breakages very seriously, especially in situations like yours where the websites are widely deployed and can't be modified. > It would be nice if there was some way > to tell Firefox to return "False" for document.watch, because then the site > would use it's fallback method - or is there any way to do that? I love this idea. @Jeff: is it possible to remove Object.prototype.watch only from web content? This way, content wouldn't see it, but addons/chrome-level code wouldn't break if depending on it. In cases where it's feature-detected, Firefox would just act like any other browser. > Anyone know if/when it's planned to be obsoleted and/or removed, because > that might be useful to pass on too. The full removal bug is bug 638054, but this is more of an Mozilla-internal bug because Firefox addons and maybe Firefox itself (what I called "chrome-level code" above depends on it). Specifically for web content, it might be better to stop exposing Object.prototype.watch/unwatch altogether. That would solve your problem and all watch-related problems.

@David I see in that bug, from a comment back in Sep 2012 > So we might as well disable Object.prototype.watch *for web content* right now. Not the first time this idea has been mooted then! No comments from anyone to say not to in that or the other linked bug to that one.

> The good news of HTMLDocument having moved to WebIDL is that... it's WebIDL compliant, so > it's possible to hack on it. This was possible in Gecko even before the move to WebIDL. If we can just make Object.prototype.watch not exist for web content, I think that would be awesome.

Just confirming issue continues in 24b1 (Linux)

I'd love to remove Object.prototype.watch for web content (everywhere, but small steps). That seems unlikely to be feasible as something we could change in a point release, tho -- Gecko-specific code paths that don't feature-test, and the like. Which means it's probably time to see if we can do some sort of temporary DOMProxy hack. I'll keep looking into this.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19) > I'd love to remove Object.prototype.watch for web content (everywhere, but > small steps). That seems unlikely to be feasible as something we could > change in a point release, tho -- Gecko-specific code paths that don't > feature-test, and the like. I'm not sure I understand. The proposal to remove Object.prototype.watch for web content is so that it remains internally and that internal components (like Gecko) can still use it. You seem to imply that there would be some Gecko-specific code paths that rely on Object.prototype.watch being exposed to web content?

No, some webpage assuming that feature/UA-detection of one thing, implies that watch/unwatch are present: if ($.browser.mozilla) { document.watch(....); // depend on that watch happening }

Can it not be a flagged/option thing - turn it off, give the option to turn it back on, so sites can modify their setup over time if they rely on this in the way you state above? The issue has also been flagged to Citrix via the support mechanisms here.

Not particularly. This is fairly deep in the implementation of the language -- it's really not easy to configure such low-level things on any sort of per-page or per-site basis. And it's unclear we'd want to, anyway, because then we're fragmenting our standards-space support, and what "Gecko" as UA means.

A later version of the Citrix code has been tried (9.3-63.4_cl) but still shows the problem, according to the department tech people.

Bleh, I really wanted to get to this this week, but other stuff kept interfering. :-\ That, plus I'm not sure offhand how we can make a totally one-off exception in this junk, solely for DOM proxies. Maybe a temporary (oh please let it be temporary) proxy hook? Or would DOMProxyHandler::defineProperty just do "the" "right" "thing" if we let execution proceed here, somehow? Anyway, this is definitely still on my radar, but I'll be out (really out, un-contactably out, to dispel any notions that this will be like some vacations I've taken) tomorrow through Monday. So I won't be able to touch it til Tuesday at earliest.

Jeff, I'm happy to make defineProperty do whatever we need here, if you tell me what the "whatever" is.

I guess hack stuff so that "watch" works on DOM proxies, in the manner it used to before? And keep it non-working on all the other objects it didn't work on before? I'd be leery of doing it |document|-specific, for fear other code is doing similar bad things with other objects.

So here's a question. Why can't we just use the normal "set watched flag" on the lastProperty() of the proxy. Yes, I know, that lastProperty() is always an EmptyShape, but I think that should be ok: we'd just slightly pessimize all the things that have that same EmptyShape, right? Or is the issue that other code wouldn't even check for watched() on non-natives?

JSObject::nonNativeSetProperty already seems to contain code for watching non-native stuff. I think it may be the case that JS_SetWatchPoint's code was written in a time when non-native meant really crazy, so all non-native got excluded. These days, it's possible removing the !isNative() check and fixing some extra stuff would Just Work. js/src/jit-test/tests/auto-regress/bug769192.js appears to be the only busted test when I just remove the !isNative() check. Conditioning the sparsifyDenseElements call in JS_SetWatchPoint on index-ness fixes that test. Watching an index of a typed array still crashes with those changes, so somehow typed arrays would still have to be excluded. Perhaps converting !isNative() to !isNative() && !is<ProxyObject>() would do the trick? I dunno, I'm well past the end of my day own. Food for thought in any case. We can't just let all native objects through here; the more narrowly we can scope the ones that pass through, and might seemingly possibly work, the better.

FYI - the developer release of IE (IE11) also fails on this Citrix system, although it fails much earlier - it cannot complete authentication. Currently that raises the possibility that two major browsers will not be able to remote access these systems. Bug was opened with Citrix, but apart from trying the latest code it appears to remain unresolved with them - I have chased for further information but none is available as yet.

Hi all - has there been any progress on this one, trying to keep work up to date as well. Many thanks.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/watch https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility

I haven't had time to figure out anything for this yet, unfortunately. (And if there's no activity in the bug, that means no one else is working on it, either.) It's possible I may not have time to do so, so I'll throw this back into the pool to not give false impressions. If I pick it back up, I'll update accordingly.

Yoshino-san: shouldn't this rather be mentioned in Site Compatibility for Fx 23?

Yeah, we're now shipping this bustage. We really need to fix this...

(In reply to Jean-Yves Perrier [:teoli] from comment #34) > Yoshino-san: shouldn't this rather be mentioned in Site Compatibility for Fx > 23? I'll add this one to the 23, 24 and 26 compat docs too :)

Actually Firefox 23 had a bunch of regressions so I'm still investigating those.

Moved the regression info to the Firefox 23 compat doc: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23

I'm looking at this again.

From the last update - can I confirm that I'm reading correctly that it won't be fixed in any future version of Firefox? If so, I can feed this back to my employer and onto Citrix.

Given comment 40, I would say your interpretation is a bit extreme. Firefox 24 already shipped, so being wontfixed for a version that has already shipped is more bookkeeping than anything else. Given that Fx25 is due to ship in two weeks, it probably won't make 25 either. 26 is still in play depending on how things play out.

Sorry, wasn't sure how to read all the + changed to - for FF25 and FF26, to someone not used to Bugzilla protocols it looks like it's been removed from consideration after the Wontfix setting.

Tracking- basically just means "we're not going to hold up a release over this" :)

Unfortunately I didn't end up finishing up this work this uplift cycle, so this won't be fixed til next release. :-( But it's top priority to finish immediately, otherwise.

I have tried one of the test builds (Linux/64bit/static) from your second build push, and that appeared to work correctly with the site (the list of applications appeared, and correctly launched WFICA and the remote program) Good work :) Thanks for all the efforts, as you push any further builds in relation to this I'll try and test each one as you need.

No typedefs in dom/, please.

Okay, I think this is good to go. The gist is that we add watch/unwatch to ObjectOps, then call those from the watch/unwatch methods. If the trap is nullptr, then the default implementation -- which throws when used with non-native objects, as has always happened -- gets used. Proxies define non-null versions of these; the implementations go through proxy handler traps. As far as proxy handler traps go, BaseProxyHandler's watch implementation throws exactly like watch does now in the non-native object case. The unwatch implementation does nothing (as currently happens if you unwatch on a proxy). Underneath BaseProxyHandler, we override watch/unwatch *only* on BaseDOMProxyHandler to fix all DOM proxies. We also override watch/unwatch on nsOuterWindowProxy, so that watch/unwatch on the global object keep working (as they do now). The watch/unwatch functionality guts, used for native objects that don't override watch/unwatch, is exposed as a friend function so that DOM proxies and the window case can use it. In the longer run we can remove watch/unwatch entirely to get rid of this fugly. The functionality is mostly forked from JS_SetWatchPoint and JS_ClearWatchPoint, which try to be more generic about how to handle invocation of a watchpoint. I think this is probably better than trying to polish this into some sort of shared thing, given that we want to get rid of watch/unwatch entirely in the mid-term. This incidentally happens to fix bug 930494. There is some trickiness to calling Shape::setObjectFlag on a proxy, which JSObject::setWatched does. Because obj->shape() of a proxy has no parent, we create a new empty shape for a proxy, rather than just creating a new shape with new base. This empty shape is stored in the initialShapes table, overwriting an existing one there -- but the association includes the object flags, which wouldn't have contained the "watched" flag before and will after, so there's no poisoning of anything existing there. So I'm fairly sure this is all safe/fine. But confirmation from someone who understands our shape/baseshape stuff (bhackett) seems justified here, particularly if this is something to backport to branches (soon, ideally, to maximize bake time before release). This isn't especially different from the patch that passed try, earlier here -- I think only in the test that got added. That test passes locally, so I think this is ready to go now as far as reviews go.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #50) > There is some trickiness to calling Shape::setObjectFlag on a proxy, which > JSObject::setWatched does. Because obj->shape() of a proxy has no parent, > we create a new empty shape for a proxy, rather than just creating a new > shape with new base. This empty shape is stored in the initialShapes table, > overwriting an existing one there -- but the association includes the object > flags, which wouldn't have contained the "watched" flag before and will > after, so there's no poisoning of anything existing there. So I'm fairly > sure this is all safe/fine. But confirmation from someone who understands > our shape/baseshape stuff (bhackett) seems justified here, particularly if > this is something to backport to branches (soon, ideally, to maximize bake > time before release). Yeah, this will work fine. Non-proxy objects run into this issue as well, since flags can be set on an object with no properties / an empty shape.

Comment on attachment 826203 [details] [diff] [review] Patch r=me for the shapes-will-still-work-fine stuff.

Comment on attachment 826203 [details] [diff] [review] Patch Review of attachment 826203 [details] [diff] [review]: ----------------------------------------------------------------- Proxy bits look great. r=me. ::: js/src/jsproxy.cpp @@ +3073,5 @@ > return Proxy::construct(cx, proxy, args); > } > > +static bool > +proxy_Watch(JSContext *cx, JS::HandleObject obj, JS::HandleId id, JS::HandleObject callable) These are silly, but I agree this is how it's done.

Try reveals I don't know a thing about how document.cookie works, so I had to futz with the test a bit to make it actually pass (given a subsequent test that expects a certain incoming document.cookie state), but other than that this was good on try: https://tbpl.mozilla.org/?tree=Try&rev=d2718cd9b8c0 Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/471db633b20c

Or...you could use the following code that even Mozilla's developer documentation links to from: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/watch As you can see, it checks if "watch" already exists. Just override it and it should work. It did for me, anyway. // object.watch if (true || !Object.prototype.watch) { Object.defineProperty(Object.prototype, "watch", { enumerable: false, configurable: true, writable: false, value: function (prop, handler) { var oldval = this[prop], newval = oldval, getter = function () { return newval; }, setter = function (val) { oldval = newval; return newval = handler.call(this, prop, oldval, val); }; if (delete this[prop]) { // can't watch constants Object.defineProperty(this, prop, { get: getter, set: setter, enumerable: true, configurable: true }); } } }); } // object.unwatch if (true || !Object.prototype.unwatch) { Object.defineProperty(Object.prototype, "unwatch", { enumerable: false, configurable: true, writable: false, value: function (prop) { var val = this[prop]; delete this[prop]; // remove accessors this[prop] = val; } }); }

Backed out for B2G desktop mochitest-1 failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/0f285b36d592 https://tbpl.mozilla.org/php/getParsedLog.php?id=30286176&tree=Mozilla-Inbound

Bleh, I guess my eyes skipped past that lone failure on first try-push. After wasting a day or so debugging it, it turns out setting document.cookie just doesn't work on b2g-desktop \o/ and that all tests that do so are disabled/skipped there. Scumbag Waldo is going to skip the new test here, too, in the interests of getting this landed to start the process of getting this on branches. Yay. :-\ https://hg.mozilla.org/integration/mozilla-inbound/rev/dad39f51b716

https://hg.mozilla.org/mozilla-central/rev/dad39f51b716 https://hg.mozilla.org/mozilla-central/rev/0ae7b31bfa6d

Updated docs: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23 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

Benjamin, it'd be nice to fix any sites that broke because of .watch not working on DOM proxies, in aurora/beta as well as on trunk, so they're fixed sooner. Doing it the way the patches here did -- any other way would be sufficiently less trustworthy that I wouldn't even consider it for branches -- requires increasing sizeof(JSClass). Is it a problem to do that, or should this just ride trains?

That is a problem on beta, yes. I wouldn't take this for Fx26. I don't think it's a problem on Aurora, because the few addons that still use JSAPI typically build from the beta1 SDK.

Straightforward backport. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 855971 User impact if declined: calling watch on DOM proxy objects won't work, which sadly some sites still do (if they detect that watch/unwatch exist) Testing completed (on m-c, etc.): landed on m-c with automated test, no complaints so far Risk to taking this patch (and alternatives if risky): biggest risk is probably that this increases sizeof(JSClass); bsmedberg says (see comment 61) he thinks we can take this on aurora, but not beta; that also gives us an extra cycle for any JSAPI-using miscreants to discover and react to our changes before release String or IDL/UUID changes made by this patch: N/A

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #62) > Created attachment 832991 [details] [diff] [review] > Aurora backport > > Straightforward backport. > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): bug 855971 > User impact if declined: calling watch on DOM proxy objects won't work, > which sadly some sites still do (if they detect that watch/unwatch exist) > Testing completed (on m-c, etc.): landed on m-c with automated test, no > complaints so far > Risk to taking this patch (and alternatives if risky): biggest risk is > probably that this increases sizeof(JSClass); bsmedberg says (see comment > 61) he thinks we can take this on aurora, but not beta; that also gives us > an extra cycle for any JSAPI-using miscreants to discover and react to our > changes before release > String or IDL/UUID changes made by this patch: N/A Would any manual test coverage from QA to try any popular add-on's help verify/test here ? If so, please add qawanted on this bug and provide some pointers so QA has this on their radar.

(In reply to bhavana bajaj [:bajaj] from comment #63) > Would any manual test coverage from QA to try any popular add-on's help > verify/test here ? If so, please add qawanted on this bug and provide some > pointers so QA has this on their radar. Just randomly trying the popular ones will likely do little good. The only addons that might be affected are ones that link against/use the SpiderMonkey in Firefox, from C++ compiled code. If QA has a list of such extensions (I assume they do, we want to hide SpiderMonkey entirely from them), testing those is worthwhile. If there is no such list, then it's probably a waste of time to test the popular addons, because it's likely very few link against/use SpiderMonkey from compiled code, given the complexity of doing so and the negligible benefits to doing it. Anyway, setting qawanted in case there's such a list -- if there isn't (which would honestly astonish me), then it probably isn't worth spending a lot of time for little gain on testing.

Updated the docs again, mentioning Firefox 27: https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23 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

Vark, can you please verify this is fixed for you now in Firefox 27 and 28?

Can confirm this as working correctly again in FF27 (Beta Channel) on Linux (64 bit). Will confirm on Windows 7 when I update that to Beta Channel FF27 (still on 26 atm)

Your testing on Linux should be sufficient. Thanks Vark.

Addit - Citrix seem to consider this a regression fault by Firefox, and not that they are using non-standard and unsupported code to detect cookie changes. The last response from their support tech was that it was FF's fault *because you listed it as a regression*. Go figure. tl;dr - My pre-broken code broke because FF took their toys away.