I'm not sure if this is handled properly, but I couldn't find any code where the right context is pushed to stack (or whatever is the right thing to do here). Because I don't have a testcase or anything, state UNCONFIRMED.
at the very least, we should confirm that the right thing does happen. jst: http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/geolocation/nsIDOMGeoGeolocation.idl#53 a js function is passed into this function. from c++, i hold a strong reference to it. when I am ready to call it back, I simply do a c++ method call. I do verify that the owner of the script still exists: http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#698 Am I safe?
Doug, when a callback, which is registered on a web page, is called, can it use Components.stack or do you get a security error? I don't have Maemo stuff installed, so I don't have a data provider, I think. So can't really test this. For event listeners we explicitly push the context of the page to stack before calling them. (I'd actually like to do it a bit different way than it is currently done, but at least the current way is safe.) If something similar is needed for geolocation, perhaps you could use nsCxPusher and push nsGeolocation::mOwner. (I hope it is the inner window) And another thing, not related to security - I believe nsGeolocation should be added to cycle collection.
Components.stack cause an assertion: Error: Permission denied for <http://people.mozilla.org> to get property XPCComponents.stack Source File: http://people.mozilla.org/~dougt/geo_stack_available.html Line: 15 Please verify that my test case is correct. for the CC issue, i filed: bug 452834
Hmm maybe Components.stack doesn't work because there isn't privileged context on stack, but whether the context is the safeContext or the context of the mOwner, dunno.
So if safeContext is always used, there isn't security problems, but chrome listeners don't get all the privileges they might want, I think.
I looked into this a bit today and we'll given that we don't know what's on the JS context stack when these callbacks fire, we'll need to push a context before calling these callbacks. After talking with dougt about this earlier today I thought we'd actually need to ensure that the geolocation code would always have access to the right inner window to be able to call these methods safely, but it turns out after having looked at that more closely that it's not necessary. After talking through this with mrbkap as well I think the safest thing we can do here is to make sure that the code that's calling the callbacks always push null onto the context stack, that way xpconnect will do the right thing in all cases, it will either use the context where the callback came from, or the safe context if it can't find where the callback came from, but never any other random context that might be on the stack when one of these callbacks happen. So dougt, IOW, you'll want to make sure you push null onto the context stack (nsIJSContextStack) before calling these callbacks, and popping (unconditionally) once you're done.
Created attachment 337181 [details] [diff] [review] patch v.1
Comment on attachment 337181 [details] [diff] [review] patch v.1 Looks good. It'd be nice to have a C++ class that would do this for you so that any early returns that might creep in here wouldn't hurt us, but unfortunately we don't have such a class that I know of (that can deal with pushing null). Write one if you want to. Either way, r+sr=jst
if we continue adding more calls out to content from geolocation (which I suspect we will when we start handling more error cases, and timeouts), i will surely refactor the js context pushing. thanks for the review!
changeset: 19106:851e9199e5f1 tag: tip user: Doug Turner <firstname.lastname@example.org> date: Wed Sep 10 08:52:46 2008 -0700 summary: Bug 452762 - Make sure Geolocation callbacks run in the right JS context r/sr=jst smaug, thanks for discovering this problem.