Steps to reproduce: 1- Go to provided URL 2- Click the "Attributes and methods of the WINDOW object" Actual results: the script function ListAllWindowAttributesAndMethods() is not executed and the js console reports "Error: uncaught exception: Permission denied to create wrapper for object of class UnnamedClass" Expected results: the script function ListAllWindowAttributesAndMethods() should normally be executed listing all attributes and methods and the js console should report nothing. Seamonkey 1.5a rv 1.9a1 build 2006020409 under XP Pro SP2 over here. Notes: - this must be a recent regression or something because I'm sure I was getting expected results no more than 2 weeks ago with a trunk build. - I searched for duplicates. About a dozen have the word "wrapper" in their summary but I can't figure out which one would be responsible for the actual results. The actual results could be (just a speculative hunch) related to fixing memory leaks or tighting up of security surrounding window object. - I get expected results with Firefox 1.5.0.1 rv: 1.8.0.1 build 20060111 - I can create a reduced testcase if needed

Possible duplicate of bug 209701

This isn't a dup of bug 209701 -- this is a regression from bug 323634. Getting window.controllers throws, since they have no classinfo.

So I think we should fix this -- breaking enumeration of properties on Window is pretty suboptimal. :( Is there a way to just mark controllers as not enumerable or something? If not, do we need to back out the patch in bug 323634 and try something different? Maybe modify classinfo to throw when getting properties of the controllers object (other than toString, possibly) from content script? Requesting blocking on the stable branches. I really don't think we want to ship this bug there.

I saw this in the weekend js tests, but was just going to work around it in the tests. Should have filed this sooner. Sorry. examples: ecma/ExecutionContexts/10.2.2-1,js, ecma/ExecutionContexts/10.2.2-2.js, ecma_2/Statements/forin-002.js.

(In reply to comment #3) > Is there a way to just mark controllers as not enumerable or something? SpiderMonkey has JSPROP_ENUMERATE, which is being used here when it should not be. IIRC DOM window properties are enumerable by default, unlike the standard class objects (Date, parseInt, etc.) in the core language. /be

Could this be what's causing bug 326279 (blocking 1.0.8)? If so we should back out bug 323634 unless there's a fix coming.

If you gave them class info limited to nsISupports, would that affect chrome? The other option that occurs to me is to make nsGlobalWindow::GetControllers do a security check and return null from content/unprivileged code.

Boris, can you help us out here? I don't think we want to live with bug 323634, but so far the only option we have to fix this is to back that one out.

This needs a lot more testing than I can possibly give it, but seems to work so far.... We should probably leave in the patch for bug 323634 for extra safety. Chrome JS should be ok. C++ might get null sometimes, in theory. :( That's where the testing comes in... Daniel, can we get some MoCo people on it? Trunk and 1.8 will need merging from this, but I figured I'd try to get review first.

Comment on attachment 211233 [details] [diff] [review] Possible 1.7 branch patch I can't see any issues given the callers: nsEditingSession (seems not to affect Midas) nsTypeAheadFind (observing domwindowopened) nsCommandManager (?; also uses IsCallerChrome) nsXBLPrototypeHandler (requires trusted event) nsXBLWindowKeyHandler (requires trusted event) nsXULCommandDispatcher (only used by JS) nsFocusController (only used by other callers) nsTextControlFrame (used in PreDestroy and also native keybindings; note that I don't see any trusted event checking in the native keybindings code, but maybe I overlooked it)

> nsTextControlFrame (used in PreDestroy Er... does that actually get controllers from a window? If so, we have a problem. Note that any C++ caller that might possibly get called while content JS is running (which means during an async XMLHttpRequest, for example :( ) will get broken by this change. So the question is whether they can all deal "well enough" with that.

(In reply to comment #11) >Er... does that actually get controllers from a window? D'oh, of course not, that was just a text search :-[ >which means during an async XMLHttpRequest, for example :( async or sync? isn't it sync that spins up an event loop? so you're saying that keyboard scrolling/clipboard will fail while an XMLHttpRequest is in progress?

Er... sync. Whatever spins an event loop while JS is on the stack without pushing a JSContext. > so you're saying that keyboard scrolling/clipboard will fail while an > XMLHttpRequest is in progress? Pretty much. But does "fail" mean "not happen", or does it mean "crash"? If it's the former, I think I'm ok with that given that sync XMLHttpRequest pretty much locks up the UI anyway with its 100% CPU thing so trying to do anything during it would be hard.

(In reply to comment #13) >does "fail" mean "not happen", or does it mean "crash"? nsXBLPrototypeHandler and nsXBLWindowKeyHandler definitely null-check. The other callers don't appear to be relevant, but maybe we should switch them from NS_ENSURE_SUCCESS(rv, rv) to NS_ENSURE_TRUE(controllers, NS_ERROR_FAILURE).

Probably (a least for midas, possibly others). I'm really not going to have time to work on this, much less test it, for about a week. If someone can please help, that would be much appreciated....

(In reply to comment #11) >Er... does that actually get controllers from a window? Actually, doesn't this issue apply to textareas/text input too?

(In reply to comment #13) > Er... sync. Whatever spins an event loop while JS is on the stack without > pushing a JSContext. Do we yet have a bug on file that makes this impossible? It violates a very old design constraint. Of course, the event queue service code came after, or grew hair, or something, and the ability to require a context push (even if the caller passes null) was lost. /be

> Do we yet have a bug on file that makes this impossible? Not yet. If someone can file, that's great; if not, I'll try to do it tomorrow when I have a more reasonable net connection. > Actually, doesn't this issue apply to textareas/text input too? Sort of, but all it'll do is screw up the one text control, which you can do in simpler ways (oninput events, say). The problem bug 323634 was fixing was that the effect persisted after you loaded a different site in the window.

So another thought... could we possibly make nsWindowSH::NewResolve not resolve controllers unless explicitly set if called from content script? Or make nsWindowSH not enumerate them from content script? Or something like that?

(In reply to comment #18) >>Actually, doesn't this issue apply to textareas/text input too? >Sort of, but all it'll do is screw up the one text control, which you can do in >simpler ways (oninput events, say). The problem bug 323634 was fixing was that >the effect persisted after you loaded a different site in the window. Right, but it's this regression that I thought would apply to textareas too.

(In reply to comment #19) > So another thought... could we possibly make nsWindowSH::NewResolve not resolve > controllers unless explicitly set if called from content script? Or make > nsWindowSH not enumerate them from content script? Or something like that? Not easily. NewResolve is only called once per JSObject (assuming the property isn't deleted), so if the first resolve comes from chrome (through an extension or whatnot that didn't use XPCNativeWrapper), then we'd expose it to content code again. GetProperty() is where this would need to go, but then again once resolved, the property getter will be found on the prototype, so no call to GetProperty() on the actual JSObject for controllers. I guess we could do manual calling of the getter for controllers in nsWindowSH::GetProperty() and conditionally return null if the caller is not chrome, but that makes overriding the property complicated... :(

(In reply to comment #15) Should we back out bug 323634 instead?

Another option would be to make XULControllers an nsISecurityCheckedComponent. Then people could enumerate stuff on the window, but things would throw when they enumerate the controllers. If I can't figure out something sane with classinfo and no one else steps up, then perhaps we should back out bug 323634...

I would have thought this would work. But I don't even see CheckPropertyAccessImpl being called on the controllers object! Which makes me wonder how the CAPS prefs could work.... If someone can figure out how to get this working, I think we should go with it. I won't be able to look at this again till Tuesday.

OK, so as I understand there were two issues in bug 323634: a) content can call RemoveControllerAt Nobody uses this. We could wrap it in IsCallerChrome() or even remove/fail it. Extensions could use getControllerAt and RemoveController. b) content can call InsertControllerAt We have two C++ callers. One is nsGlobalWindow, when it creates the controller. It could be changed to append the controller. The other is editor/Midas; is there some fix we can use allow us to wrap it in IsCallerChrome()?

So at this point, I think we should back out the patch we used for bug 323634 and go with the original CAPS changes I proposed on the branches. That should be safe enough, since I doubt we'll change nsIControllers on the branches. On trunk we should think of better ways to do this, of course, probably in this bug.

reverting the 323634 change and bailing out of nsXULControllers::RemoveControllerAt and InsertControllerAt if (!nsContentUtils::IsCallerChrome()) seems to fix both bugs. This would have the same callback issues as the proposed nsGlobalWindow::GetControllers change, but limits the risk to two routines that aren't called much, and aren't likely to be called in a callback. I happy to go with the caps change instead, though, it seems to work fine.

> but limits the risk to two routines that aren't called much, and aren't likely > to be called in a callback. InsertControllerAt is called when a page toggles on designMode. So just putting IsCallerChrome() in it would break Midas unless we also did some other magic (eg pushed a null JSContext on the stack in editor init before messing with controllers).

Per discussion w/bz fixing this one on old branches (aviary101/moz17/moz180) by backing out the fix for bug 323634 and checking in the caps approach.

verified with: Windows: Moz - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214 Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214 Firefox/1.0.8 Macintosh: Moz - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060214 Firefox/1.0.8 Fx - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13) Gecko/20060214 Firefox/1.0.8 Linux Moz - Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060214 Fx - Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060214 Firefox/1.0.8

Thanks to jst for catching the doCreate thing.

Comment on attachment 211915 [details] [diff] [review] Patch for trunk and 1.8.1 sr=jst

v 20060217 winxp 1.8.0.2 debug build and on 20060217 win/linux/mac nightlies where the js tests no longer fail with this error.

Comment on attachment 211915 [details] [diff] [review] Patch for trunk and 1.8.1 r=mrbkap

Fixed on trunk.

Fixed on 1.8 branch