Since the the beginning of time we've always shared JS wrapper for our native DOM objects between content and chrome, and this has always caused security problems, some we've found, others I'm sure we haven't. Our approach in tackling these problems so far has been to use Caillon's XPCNativeWrapper() helper in JS to reach the actual DOM properties from chrome and not properties the page set. But that means that developers need to be aware of this problem, and lots are, but not all, and we're all human so occatinally we forget to use the wrapper and we introduce potential security problems. Not sharing JS wrappers between content and chrome is fairly easy, but it has its consequences. Like XBL bindings must not be attached more than once just because a content DOM object is accessed from chrome. And not only that, but XBL properties etc that were attached to the content DOM object won't be visible to chrome. I imagine that it's ok for us (at least from a technical point of view) to make content XBL unreachable from chrome, and that's what my patch does, and it doesn't appear to break anything obvious (that I've seen so far), but it may break other apps, who knows... I'll attach a patch for others to look at that makes us not share JS wrappers between content and chrome (but we'll still share among chrome windows, and among content windows, of course). Please comment and share thoughts on this issue. I believe that at some point in the future we're going to have to make a change like this, and the sooner we do that the easier it'll be for us. But yeah, it would've been even easier to do this 3 years ago... We could, though it won't be easy, provide a mechanism for chrome code to specify on a per JS stack frame basis (or whatever) that it wants to temporarily share JS wrappers, but I don't know yet that we'll need to do that. Thoughts?

Oops, that can't be the right patch -- it looks like a s3kr3t plugindir one I reviewed recently! ;-) /be

The change, as described, seems pretty reasonable to me. The XBL thing may bite some extensions, but the really popular ones that I can think of that bind XBL to content (flashblock, specifically) should be ok...

Yeah, duh, that was the right patch filename, but wrong directory. This is what I meant.

Does this include things like frame names reflected into the window object? This might break a few odds and ends such as the XUL directory listing.

If chrome accesses an object and sets properties on the wrapper, can content later access those properties? I can't quite tell from the patch, but the code in the patch seems asymmetric wrt chrome and content...

What about things like DOMI? The JS object browser in DOMI would be seriously affected by this, correct? I think that we should consider doing this right (in the future, not here) and have a wrapper per-security-domain, such that www.foo.com and www.bar.com would get different wrappers for the same DOMWindow object and any other objects that they could possibly both get access to. In that case we could probably get rid of a lot of the restrictions that we have historically needed on setting custom wrapper properties. --BDS

If it turns out that we do need to be able to access the content-wrapper from chrome, maybe one option would be to have some XPC method that allowed you to manually fetch the content-wrapper given a chrome one. Rather then doing it per window or some such which seems like it's easy to forget that you've done for a certain window.

To clarify comment 6, if content can access the chrome-wrapper (say chrome ends up doing JS access first, then content does JS access), then we still have a problem...

(In reply to comment #6) > If chrome accesses an object and sets properties on the wrapper, can content > later access those properties? I can't quite tell from the patch, but the code > in the patch seems asymmetric wrt chrome and content... Yeah, the code is asymmetric, but we want that. We only ever want to "prevent" chrome from seeing JS wrapper changes that were done in content, the other way around we don't want to prevent anything, but in reality all access is prevented the other way around since content can generally never access chrome. In the odd case where content code has requested UniversalBrowserRead/Write then IMO content code *should* be able to see chrome wrappers n' all, since there's nothing we want to hide going that way, IMO. (In reply to comment #7) > What about things like DOMI? The JS object browser in DOMI would be > seriously affected by this, correct? Correct, this screws DOMI in a big way. Need to think about what to do there... > I think that we should consider doing this right (in the future, not here) > and have a wrapper per-security-domain, such that www.foo.com and > www.bar.com would get different wrappers for the same DOMWindow object > and any other objects that they could possibly both get access to. In > that case we could probably get rid of a lot of the restrictions that > we have historically needed on setting custom wrapper properties. I don't think so. This is not about protecting one domain from seeing or not seeing something in another domain, this is strictly about making chrome development safer. And no matter what wrapper is created for any given domain, there's much more to protect than what's on the wrapper, most of what we're protecting is in the underlying object that's the same no matter what wrapper is used.

i gave bz a list of things i expected to be screwed when i first saw this bug reported, (i didn't list domi, i presumed someone else would cover it for me), domi-sidebar, xul error pages (bz said biesi would check on it), my company's product, my previous company's product, loading chrome in a tab (chrome://navigator/content/, chrome://messenger/content/, chrome://chatzilla/content/, or ...), loading *any* chrome url in winEmbed/mfcEmbed/gtkEmbed/..., unfortunately i don't have the list i gave bz, so this is from memory :(. i would hope that none of internal cases listed above are broken when someone commits a 'fix' for this, and i expect that the fix include apis which will allow other embedders to easily unbreak their modules by following the examples from seamonkey/winEmbed. probably a notification message to the docshell owners about a transition to a trusted content location in a 'content' region so that the owner can decide to change the location from 'content' to 'chrome' (and similarly for transitioning the other way), ideally such an api will not have the problems that we've encountered for https transitions, but .... :)

timeless, I think you have the wrong bug... This isn't the bug about dropping chrome privs for chrome:// uris in a content docshell.

So I thought some more about how to do this w/o breaking DOM Inspector etc, and how to provide a way for chrome to access the content wrappers in the rare case when it really *needs* to, which AFAIK won't ever happen in our apps (except in the DOMI case). Ideally we'd have a privilege-like mechanism that we could use in chrome to enable wrapper sharing only in a given scope, but unfortunately that'll be *really* hard to do w/o a lot of suck. We'd need to make XPConnect aware of this and make it check for this *every* time a wrapper is accessed from JS, even in the case where the wrapper already exists (which needs to be as fast as possible, now it's basically just a locked hash lookup). I don't want to mess with that code, it's performance critical. And approaches like providing a method that returns the content wrapper to chrome only goes so far, since any code (read XBL methods/getters/setters) on the content wrapper won't work as expected when accessed in the content scope, even if it's accessed through the right wrapper. So the alternative to me looks like a two-sided approach. 1: For apps like DOMI (and Venkman?) we'll add a global function on chrome windows that it can set that will from then on enable XPConnect wrapper sharing (of wrappers that have not been created thus far) between this chrome window and content windows. This function can only enable sharing, disabling it gets tricky (since XPConnect is caching the already created wrappers etc, so a predictable on/off is not easily doable). 2: For trancient access to a content wrapper from chrome code we'd introduce a new eval()-like method that would evaluate an expression in the given scope, and on the JS context of the given scope (i.e. pass it a content window and the code will be run on the content windows context, just as if it was accessed from the content window). The JS that is run in this method runs with the priveleges of the given scope, i.e. no elevated rights for the code that runs from within this new uber-eval(). Maybe this should be evalInContext(), or something like that, and I think we need a way to not only eval a string of JS, but also a way to call methods (and access properties) on objects from chrome, that means we'd need a way to pass arguments here. Anyone see anything obviously wrong with an approach like this? Or is there an easier approach here that I'm just not seeing? I'll start hacking on this, yell if I should stop.

i'd argue domi and venkman should probably fall into #2. i don't want someone to write a page that waits to attack a domi or venkman user. the code should be careful. and i do believe i meant this bug when i commented in it, i hadn't read or heard about the other bug at the time. and my implementations of all these things has always involved exiting the local <browser/> region and sticking things back into the <xul:window/> or vice versa.

(In reply to comment #11) [...] > xul error pages (bz said biesi would check on it), I can't imagine how this would break XUL error pages... [...] > loading chrome in a tab (chrome://navigator/content/, > chrome://messenger/content/, chrome://chatzilla/content/, or ...), nor would I expect any of these to break. > loading *any* chrome url in winEmbed/mfcEmbed/gtkEmbed/..., I can't see how this would change things in any way for embedders. > unfortunately i don't have the list i gave bz, so this is from memory :(. > > i would hope that none of internal cases listed above are broken when someone > commits a 'fix' for this, and i expect that the fix include apis which will > allow other embedders to easily unbreak their modules by following the examples This shouldn't change anything for embedders, we're only changing how *chrome* JS sees content JS objects, embedders are in no way impacted by that that I can see. > from seamonkey/winEmbed. probably a notification message to the docshell owners > about a transition to a trusted content location in a 'content' region so that > the owner can decide to change the location from 'content' to 'chrome' (and > similarly for transitioning the other way), ideally such an api will not have > the problems that we've encountered for https transitions, but .... :) I guess I'm not getting what you're saying here...

(In reply to comment #14) > i'd argue domi and venkman should probably fall into #2. i don't want someone to > write a page that waits to attack a domi or venkman user. the code should be > careful. The whole point of DOMI and Venkman is to see what's on the webpage, what's on the JS objects etc, this change won't open up any new security exploits that we don't already have, so noone gets more vulnerable due to this change. And this is merely about preventing content from shadowing DOM properties from chrome, no matter what a webpage does should ever give content code any elevated priveleges, and if there's ever a bug that does that, this won't save us. I'd be all for only doing #2, but I don't have (nor has anyone else I know) the resources to make DOMI and Venkman use it, so #1 makes a whole lot of sense to me.

my version of xul error pages reached from a chrome page into a content page to play with the pretty error.

(In reply to comment #17) > my version of xul error pages reached from a chrome page into a content page to > play with the pretty error. Well then your version of XUL error pages is free to use either of these approaches here to get around any possible problems. And the only such problems would be if your code relied on XBL properties/methods in the content page (which seems rather far-stretched to me).

I claim we want this for the 1.8b2 milestone, to iron out any problems so it ships in Firefox 1.1. /be

While testing my extensions with this (for related tracking bug 289231), I noticed something unusual. My extensions broke, but *not* when they were triggered by the first page load in a new tab. Reloading a tab, or visiting some other site first in the new tab did result in breakage. Since breakage is probably the (unfortunately) expected result of this bug, I wonder if the current patch is missing some special case for a new tab. FYI, whoever is testing this. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050405 Firefox/1.0.3

So, just to be clear: . If chrome-code stores a property/object on a contentWindow, will the property + wrapper be lost as soon as the chrome-function returns? . Presently, Adblock (among others) is broken, because it stores a per-page list of all blocking-metadata in each webpage's scope. It does this by adding an array on contentWindow._AdblockObjects, from chrome. The array now "disappears". If this is expected breakage, with no possible workaround, then I have to ask: what kind of security problems, besides "occatinally we forget to use the wrapper" is this patch really addressing?

(In reply to comment #22) > If this is expected breakage, with no possible workaround, [...] The patch is incomplete and there would most definitely be a workaround before this was landed for real. But since no existing code is written to use the workaround it's valid to test for breakage with this half of the patch. And boy, have we seen breakage! Far more than I expected, though we knew it was risky. > then I have to ask: what kind of security problems, besides "occatinally > we forget to use the wrapper" is this patch really addressing? Just that. The current model is the chrome author needs to remember to get content properties safely each time. The intent is to switch to a safe default and require the places that really mean it to explicitly code "get me the javascript property". In that model the downside of forgetting is you get nothing, as opposed to the current downside of forgetting being a potential security issue depending on how the data is used. Obviously this isn't going to fly on the branch. It'll have a bumpy landing on the trunk, too :-(

I vote to kill this patch entirely, before it does more harm. . Breaking the Intuitive + useful scoping between chrome and content, just because a few people "forget to use the wrapper", is a very poor choice. Perhaps borderline absurd. It: a) throws numerous extensions + descriptions + tutorials out the window; b) breaks a statistical majority of code against the future minority gain; c) inreases the chrome/extension-dev learning curve; d) doesn't appear qualified by any real security issues (unless forgetfulness counts). . Caillon's wrapper works. So why not leave well-enough alone?

My viewpoint is always about keeping the bar low for people trying to learn the scripting layer around Mozilla, so I put my "struggling learner" hat on while reading this. I vote against this fix because it just makes Mozilla-the-platform harder to learn and harder to use. Can anyone point to a fixed and a not-fixed script example for me to munch on, even if it's old code? - N.

This patch makes it easier, not harder, to write good code for the Mozilla platform. At the moment it is very easy to write highly insecure code, and learning how to work around it is hard. This patch makes it easy to write secure code, and it would only be hard to learn how to write potentially insecure code.

(In reply to comment #24) > just because a few people "forget to use the wrapper" You clearly have no idea what the scope of the problem is. I'd estimate that the wrapper is used in no more than 30-40% of the places it _should_ be used in in well-audited Firefox code (all of which postdates it). In most extensions it's not used at all. In SeaMonkey code that predates the existence of the wrapper it's rarely used. > a) throws numerous extensions + descriptions + tutorials out the window Yes, this is a problem. But looking at the code that is broken by this wrapper (various extensions mostly), it's broken because it's susceptible to being exploited by the web page. In other words, this patch is doing exactly what it should be -- preventing people from writing extensions with security holes, which is what they are doing now. > b) breaks a statistical majority of code against the future minority gain; It would fix about half a dozen existing open bugs that I know of. It would allow vast simplification of the code in both Firefox and extensions that care about security. It would keep extensions that don't care about security from being exploited. I'm not sure where you got the idea that there is "future minority gain". There is a very distinct gain here, if nothing else in a safer browser for our users and a lot less engineering time spent on whacking this issue in every single place it shows up. If it's of any interest to you, I saw another dozen or two places in Firefox context menu code that aren't using the wrapper and should be while I was looking up the code example for Nigel. This is code that's been audited to death already and uses the wrapper stuff extensively. > c) inreases the chrome/extension-dev learning curve; Frankly, no. What it does do is make it a lot harder to write extensions or chrome that have security holes by design. It makes it _easier_ to write safe code. That's the whole point, in fact. > d) doesn't appear qualified by any real security issues (unless forgetfulness > counts). Forgetfulness, just like any other human factor, is a real security issue. Having security policies in place that are easy to follow is a much better security architecture than having hard to follow ones (the latter is what we have now). (In reply to comment #25) > I vote against this fix because it just makes Mozilla-the-platform harder to > learn and harder to use. This seems to be a common misconception. Can you, or someone else, give a reasonable example that is: 1) Harder to do with this proposal. 2) Doesn't open one up to security holes. ? > Can anyone point to a fixed and a not-fixed script example for me to > munch on, even if it's old code? I'm not sure what you mean by "fixed" and "not-fixed". I can point you to an example of safe code as it has to be written now and code that would become safe with the proposed patch. The relevant code is in the contentAreaClick in browser/base/content/browser.js Current code (I left out some parts not needed to illustrate the point; the density of XPCNativeWrapper calls is not quite this high in the original code, because there's other logic in between them): wrapper = new XPCNativeWrapper(linkNode, "href", "getAttribute()", "ownerDocument"); target = wrapper.getAttribute("target"); if (!target || target == "_content" || target == "_main") { var docWrapper = new XPCNativeWrapper(wrapper.ownerDocument, "location"); var locWrapper = new XPCNativeWrapper(docWrapper.location, "href"); if (!webPanelSecurityCheck(locWrapper.href, wrapper.href)) return false; } Code as it could be written: var target = linkNode.getAttribute("target"); if (!target || target == "_content" || target == "_main") { if (!webPanelSecurityCheck(linkNode.ownerDocument.location.href, linkNode.href)) return false; } Note the vastly improved clarity and ease of authoring of the code.

Stupid question. Looking at the "broken" extensions, so we know if there are other ways to do what they are trying to accomplish? Or are some of these extensions relying on things that will never work?

See comment 13.

Boris: Unfortunately, since you did mention comment13: . Toggling wrapper-sharing at the window-level will only result in the popular extensions turning it on, making things "insecure" again. . As for evalInContext(), throwing away calling-privileges would kill access to functionality like: setting an in-page eventlistener that runs privileged code. Or: clobbering a native deliberately, from chrome, with an intelligent getter that returns the native code for all chrome-calls (but something else, for in-page calls). . After considering the contentAreaClick example: . Why not flag everything set by in-page code, at the time it's set. Whenever a flagged / potentially-clobbered value is accessed: a) the code in charge of retrieval would check for a "preferNative" flag in the calling scope-chain -- allowing window-level or isolated toggling; b) if the flag is found, the retrieval code would lookup the native value (if any). This would permit existing extensions to continue unbroken (most don't need to access in-page clobbers), while securing the greater areas of concern. The DOMI would simply need the "preferNative" flag, at window-level; something a scripted-overlay could externally lend.

make that: "preferLocal", sorry. Native-lookup would be default.

> Toggling wrapper-sharing at the window-level will only result in the popular > extensions turning it on, making things "insecure" again. That's the choice of those extension authors. They're free to write insecure code if they really want to (and others are free to publicise this fact, of course)... Note that the window-level thing is there for cases when explicitly annotating every access would be too hard. It's NOT the preferred method of doing things. > throwing away calling-privileges would kill access to functionality jst, would the proposal actually remove that functionality? I'm not up enough on my XPConnect and JSeng to tell.... For similar reasons, deferring to brendan and jst on the counterproposal in comment 30.

My proposal to jst this morning is this: Give chrome and content separate wrappers, but make a one-way binding from chrome to content whereby any property set on a chrome wrapper for native instance x sets the same property on x's content wrapper -- but not vice versa (content sets a property on its wrapper for x, nothing happens to chrome's wrapper for x). Comment 30 describes something in terms that are hard to implement. Where is this flag, and how does trusted code find the "native" value that was clobbered? In the prototype object? That too could be overwritten. You end up needing two wrappers for each native instance x, in order to avoid a pigeon-hole problem. Wherefore my counterproposal. /be

Brendan: Since your proposal likewise solves the breakage, it sounds good. . Per your question: Caillon's XPCNativeWrapper uses Components.lookupMethod(), to determine what a wrapped-native's original property/method was. See xpccomponents.cpp#2004 -- I was thinking my proposal would do the same. The flag itself could just be a variable (__preferLocal__), which would make checking up the chain extremely simple; all top-level contexts (window) would have it default-defined on their prototype: chrome:false, content:true. . Also: with(){..} statements seem the direct equivalent of jst's evalInContext(), from comment13. Could you clarify why usage is more recently deprecated? It's a very useful thing.

.append (for completeness): The other flag -- "potentially clobbered / in-page" -- would be set by the backend, at parse-time, and not script-accessible. Member lookup would always check for it.

(In reply to comment #30) >As for evalInContext(), throwing away calling-privileges would kill access to >functionality like: setting an in-page eventlistener that runs privileged code. Wouldn't event handlers added with addEventListener run with their original privileges (i.e. those of the window in which they were compiled)?

i was explicitly asked to determine whether jst's proposed patch breaks our app, the answer is yes. the spider is 100% hosed since it starts out as chrome and then pokes through looking at the web page content to get things like document.links. (and yes, we're aware of the security concerns, for the most part it isn't an issue for our app, although we do use xpcnativewrapper and were screwed when it was moved.) i haven't tested the rest of our code, i decided to take a detour and see how i can work around this security measure (it's possible). i'm 99.9% certain the code will also break our other modules, as they all behave the same (some use xpcnativewrapper more than others, but they all start out as chrome and spend their time interacting with content).

So a Web page could trick your spider into running arbitrary code? That seems like something you _should_ be concerned about.

what i'm concerned about is whatever my manager and his manager tells me to be concerned about. note that as we're a commercial product, we don't license people to spider random web pages, only their own apps, as such, if their own apps decide to attack mozilla, well... fine then. and no, it's not ideal, yes we're working on it, but our app is not a generic web browser, it has a special domain where the rules and realm work differently.

rue: we're working on something sort of like what you sketched, but not exactly. Regarding with statements, they are deprecated because in function f(o) { with (o) return x; return null; } f({x:42}) returns 42, while f({}) returns null -- but setting a global x = 42 before calling f({}) also returns 42. Only at runtime do we know what names bind to which properties of what object. One way to keep with (for utmost compatibility) is to extend JS (a la VB, IIRC) to allow with users to specify that they mean "in the object named by this with statement": function f(o) { with (o) return .x; return null; } (Obviously a contrived example, since it's so short.) With is not the same as evalInContext, however, since it extends the scope chain whereas evalInContext replaces the scope chain. /be

ok, fwiw domi has the ability to violate the new model jst created in the preceding patches, i can make my code use that approach.

(In reply to comment #36) > (In reply to comment #30) > >As for evalInContext(), throwing away calling-privileges would kill access to > >functionality like: setting an in-page eventlistener that runs privileged code. > > Wouldn't event handlers added with addEventListener run with their original > privileges (i.e. those of the window in which they were compiled)? It depends on where they were compiled. If evalInContext is like eval, you pass it a string containing JS program source. If that contains the listener function it will be compiled in the content context, and have content privs. Adding a chrome-loaded (therefore chrome-privileged) function as a listener would work, but would not use evalInContext by itself since there would be no property path to the function. The chrome script would have to set the function as the value of a content (wrapper) property first, then evalInContext("thatContentNode.addEventListener(thatContentNode.listenerRef)") or some such. Ugly. We're trying now to avoid separate wrappers, to preserve compatibility while restoring by-default security, more or less along the lines rue sketched. The devil is in the details, though. /be

(In reply to comment #37) > i was explicitly asked to determine whether jst's proposed patch breaks our > app, the answer is yes. the spider is 100% hosed since it starts out as chrome > and then pokes through looking at the web page content to get things like > document.links. document.links should work just fine even with separate wrappers so I don't understand why that should break your spider. The only thing that is affected is *js* set properties on native objects. So the only way it'd be affected is if the webpages you're crawling overrides document.links and makes it point to some custom js-object. Is that really what you are doing?

try it yourself, use domi, pick any page with at least one link. 0. open navigator to a page w/ at least one link [perhaps verify that it works, javascript:void(alert(document.links.length))] 1. open domi 2. in domi, file>inspect window>navigator 3. in the left pane, select document-dom nodes 4. expand this path: #document/window/hbox/vbox[2]/hbox/tabbrowser/xul:tabbox/xul:tabpanels/xul:browser 5. in the right pane, select object-javascript object 6. expand this path: Subject/contentDocument/links/length with the patch from this bug, the result i get is 0. note that there is a way to get domi to give you the other answer, and if necessary i will use it and complain when someone breaks it. (fwiw links.length is the only thing that's broken, contentDocument.documentElement.innerHTML pretends the document is empty, among other properties that are relatively unhappy but willing to lie.) i was starting to investigate what it would take to do what domi did, but venkman crash :)

Comment 44 needs investigation, but it doesn't prove a general problem (note that we are not trying to make jst's separate-wrappers patch in this bug land for 1.0.3, so don't fret). In fact it explicitly says something odd is going on with document.links.length. Timeless: are you gonna try to debug again? /be

Actually, I have a related question. In our proposed setup, how do array-like properties on node lists work? So if I do document.getElementsByTagName("head")[1], say? Those aren't on interfaces, really...

bz: [] is a shorthand for .item(...). the same had to apply to XPCNativeWrapper before it.

Thanks bz: I see how retaining simple-but-currently-insecure syntax plus a fix yields simple-secure-syntax at the cost of less access to content js properties and/or more complex syntax with no cost. Re comment 33: if s/set/set or get/ applies, what happens if you watch() a content property from the chrome? More generally: I don't believe content-set js properties are interesting only to the special cases of DOM Inspector, spiders and compiled apps. Chrome scripting is useful as a general purpose macro-like language when it manipulates living, running Web applications, just as VBA-for-Excel does for spreadsheets containing formulae. That large class of chrome uses shouldn't be made obscure or impossible to do. - N.

jst just checked in branch patches for bug 289074 that implement what Brendan was talking about yesterday in this bug. Initial testing seems to show the extensions broken by attachment 179766 [details] [diff] [review] are not broken by the new patch (AdBlock, chatzilla, dom inspector,...). Both Firefox and Suite 1.7.7 nightlies are available for testing, look for today's evening builds.

How much more do we expect here for 1.8b2? We're coming into the end game for b2 so we need to make quick work of remaining issues or push them off to 1.8b3.

We need to fix this for 1.8b2. Nigel's right, we shouldn't break getting content-set, non-overriding DOM properties from chrome -- we just need to sandbox them rigorously. To do that without adding the cost of a thunk per content-set getter, setter, and method, we should use jst's separate wrappers approach, with a magic mirror between the chrome and content wrappers, such that content can't affect chrome's wrapper, and chrome runs content-defined getters/setters/methods in the content scope, on the content context (necessary for native method invocation to result in the content principal being found by caps code). The magic mirror will need to thunk methods, but only when chrome script gets a content-set native method on the content wrapper. Content-set scripted methods carry their principals in their script if uncloned function objects, or in their immutable scope chain if cloned function objects. More tomorrow. /be

> content-set, non-overriding DOM properties from chrome -- we just need to > sandbox them rigorously. Just to be crystal clear: we must bypass, not sandbox, content-set DOM-overriding properties. /be

Work in progress, does not compile even (nsTHashtable.h and friends are not so friendly to opaque struct types). But the idea is not only to wrap content native method getter/setter/values safely, but (this part is not even started here) to set content property foo when the corresponding chrome wrapper for the same DOM native content object has foo set. /be

(In reply to comment #53) This seems reasonable to me, and with the code to set properties across scopes I think this could go in with a really low risk of breaking extensions. As for the hash usage, you could just use pldhash directly as there's already other code in nsDOMClassInfo.cpp that does that...

And testing. Please help. jst, I see that nsGlobalWindow::OnFinalize assertion. Do I need your Aviary version, or some part of it? /be

This patch picks up the old JS_SetObjectPrincipalsFinder API I introduced in late 2003 when working with caillon on eval-based native function exploits, and revises it incompatibly to locate the findObjectPrincipals callback in the JS runtime, not in a particular JSContext. Besides economizing and matching the other principals callback (for XDR), this guarantees coverage: I am worried about a JS component loading and running on a "safe context" that doesn't have a findObjectPrincipals callback configured. This patch beefs up the belt-and-braces checks in obj_eval and script_exec to throw an invalid indirect call error in any case where the eval function object, or the script object, has object principals different from its calling script's subject principals. But the main change here is to nsScriptSecurityManager::GetFunctionObjectPrincipals, so it no longer skips native frames when walking the stack to find subject principals. Skipping natives is a very old design decision (Netscape 4 at least, possibly 3), which assumes that natives are trusted code that have no trust identity, as scripts (with their codebase or certificate principals) do. This assumption is flawed, as the eval and script object exploits recently fixed demonstrate. Evil script can store a native function object reference somewhere (in the DOM, or in a JS object created to masquerade as a DOM subtree), knowing that the native will be called in a certain way, and run with chrome scope and principals. We've been patching the symptoms of this general flaw in caps for 1.0.x, but this fix addresses the root of the problem. I'm attaching optimistically, still recompiling and testing, but an earlier version of this patch that didn't touch jsapi.h fixed the known testcases. /be

Comment on attachment 182537 [details] [diff] [review] no unshared wrappers, but general native function security (prerequisite to any future patch) This appears pretty sane, and has some portions pretty similar to a patch that I think I worked on with you a while ago (not sure why that fell on the floor). r=caillon

Comment on attachment 182537 [details] [diff] [review] no unshared wrappers, but general native function security (prerequisite to any future patch) sr=jst

Comment on attachment 182537 [details] [diff] [review] no unshared wrappers, but general native function security (prerequisite to any future patch) >Index: caps/include/nsScriptSecurityManager.h > // Returns null if a principal cannot be found. Note that rv can be NS_OK > // when this happens -- this means that there was no script associated > // with the function object. Callers MUST pass in a non-null rv here. > static nsIPrincipal* >- GetFunctionObjectPrincipal(JSContext* cx, JSObject* obj, nsresult* rv); >+ GetFunctionObjectPrincipal(JSContext* cx, JSObject* obj, JSStackFrame *fp, >+ nsresult* rv); Can you document what fp is used for, if it can be null, etc.? >+ // No chaining to a pre-existing callback here, we own this problem space. >+ ::JS_SetObjectPrincipalsFinder(sRuntime, ObjectPrincipalFinder); Should we warn if we find a pre-existing one? (And if we exit with one that's not us, perhaps?) > * All eval-like methods must use JS_EvalFramePrincipals to acquire a weak > * reference to the correct principals for the eval call to be secure, given > * an embedding that calls JS_SetObjectPrincipalsFinder (see jsapi.h). > */ > extern JS_PUBLIC_API(JSPrincipals *) > JS_EvalFramePrincipals(JSContext *cx, JSStackFrame *fp, JSStackFrame *caller); I'd like a Get in that name to keep me from reading "Eval" as a verb, but that's not what this patch is about. r=shaver on the JSAPI stuff.

I checked in attachment 182537 [details] [diff] [review]. /be

Brendan, sorry, but this Checkin have caused a serious Regression. I am using Windows-CREATURE-Tinderbox-Build 20050400 at moment. First Regression: Unable to install *.xpi with this Build. JS-Console throws: Error: uncaught exception: Permission denied to create wrapper for object of class UnnamedClass and XP-Install is aborted without installation. Second Regression: At Mozilla Startup JS-Console throws: Error: uncaught exception: Permission denied to get property UnnamedClass.Constructor without doing anything exept starting JS-Console. Third Regression: MailNews is unuseable at Moment, Thredpane is complete grey. Startup MailNews give a lot of Errors in JS-Console, e.g.: Error: uncaught exception: Permission denied to get property UnnamedClass.Constructor and using Mnenhy installed in Profile with older Build: Error: goMnenhy is not defined Source File: chrome://mnenhy-headers/content/mnenhy-headers-msgHdrViewOverlay-loader.js Line: 79 Error: goMnenhy is not defined Source File: chrome://mnenhy-headers/content/mnenhy-headers-msgHdrViewOverlay-loader.js Line: 49 and some more. Also Error: Error: uncaught exception: Permission denied to get property HTMLDivElement.nodeType was shown up. Formerly I have used Windows-CREATURE-Tinderbox-Build 2005050322 without this Patch, and it worked fine. Can you please backout this or fix it soon? TIA.

Add Screenshot from broken (regressed) Threadpane in MailNews

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050504 Firefox/1.0+ 02:16pdt this checkin is causing serious trouble If you try to install any extension FF locks up the moment the filecheck starts

I do not know if all possible regressions should be reported here. Opening external links in a new tab is also broke, it opens a blank new tab. (Options -> tabs -> Open links from other applications in -> A new tab in most recent window)

Apparently some of our UI needs to call a content native from chrome, yet have chrome privileges. I haven't debugged to see why. This means we can't use object principals for content natives, for now. It also says we have to change *something* -- we can't have it both ways. /be

I checked in a change to nsScriptSecurityManager.cpp to make it match the branch patch (which already landed on AVIARY_1_0_1...BRANCH and MOZILLA_1_7_BRANCH). I changed nsScriptSecurityManager::GetFunctionObjectPrincipals to skip native frames again. I also re-ordered the tests to avoid preferring cloned function object principals to eval or new Script principals (reviewers take note). That was an independent bug in yesterday's checkin, but not (I believe -- need to test more) relevant to the regression. Testing more today should show why our chrome counts on calling content-scoped natives with chrome privileges. XBL seems implicated. Builds should be restaged, I'll talk to people who can help do that in a bit. /be

Ok, the profile manager works again.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050504 Firefox/1.0+ 10:02pdt build Adblock still causes Error: [Exception... "'adblockEMLoad: TypeError: adblockEntry has no properties' when calling method: [nsIDOMEventListener::handleEvent]" nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)" location: "<unknown>" data: no] but appears functional. everything else seems to work again again

comment 68 is an unrelated problem caused by changes in Extension Manager's DOM. The exception appears in earlier builds as well.

The initial checkin caused bug 292864, bug 292860.

(In reply to comment #70) > The initial checkin caused bug 292864, bug 292860. And bug 292871.

Also bug 292902 I guess.

jst should look too. In thinking about native function hazards more deeply, it's clear that if we have only one bit (0 for scripted, 1 for native), we don't have enough information to decide whether to use the native function object's scope to find its principals. (We do have enough bits to decide how to handle the scripted case, of course; that is easy because scripts carry their own compiler-sealed principals, and cloned function objects have scope-sealed principals that override their prototype's script's principals.) But if the bit is 1, i.e., we have a native frame, we can't just skip it as we've done for ages -- that leaves us open to attacks involving dangerous natives. Nor can we use the native function object's scope-sealed principals, either -- that obvious broke a bunch of XBL-ish stuff yesterday. We need another bit. That bit is the condition (native function is a bound method and this frame is a call of it on a different initial |this| parameter than the native function object's parent [to which it is bound by native code using the JS API]). Both LiveConnect and XPConnect use the JSFUN_BOUND_METHOD flag to force |this| to bind to a reflected method's parent. That's good, it makes methods able to know the type of their |this| (obj in the JS API JSNative signature) parameter statically. But references to dangerous methods, though the methods are |this|-bound to their immutable parent scope objects, may nevertheless be extracted and called on a different |this| param. The JS engine will override that nominal param with the method's parent, and do the call. This patch adds a frame flag, JSFRAME_REBOUND_METHOD, that the JS invocation code sets in this case, and only this case (whether the function object is scripted or native, but that's just an aside). This patch then modifies nsScriptSecurityManager::GetFunctionObjectPrincipal to test, when about to skip a native frame, that the frame is not for a rebound call to a bound method. If it is, then the object principals for the method are returned, instead of the frame being skipped in search of a scripted caller. This defeats all native attacks using evil bound methods. Unbound methods (JS native functions by default are unbound) must be individually protected from misuse. I've done that in the last patch, on the trunk, for eval and new Script. I don't know of other dangerous unbound native methods, but welcome comment. After this patch is tested and goes in (I expect it will not break anything but exploits), we still have to consider the shared vs. split wrapper issue about which this bug is nominally concerned. /be

Comment on attachment 182713 [details] [diff] [review] distinguish rebound method calls from unbound and default-bound calls jst pointed out a flaw that can be fixed -- not sure why my testing didn't show the same hole he saw... new patch in a minute. /be

Two comments, of which the last one is mostly me brainstorming and might not be right at all :) Using the native function object's scope-sealed principal really feels like the best solution so it sucks that we apparently can't use that. Would be good to investigate what exactly is hindering this. What I feel a bit uneasy about is that we detect the case that the |this| pointer was changed. When in reality the danger isn't that the |this| pointer is tampered with but that we're tricked into calling a native function without knowing. Could we make it so that when a native function is set as a member we flag the _function_ as REBOUND and then always give the frame calling that function the treatment you are now no matter if the parent was changed or not. Ideally we should really flag the member rather then the function, but I'm not sure if that's possible.

shaver can do second sr, I'd be delighted. jst's point was that one of the attacks rebinds the bound method in the same object that it is bound to already, so my parent != thisp check was not enough. This patch checks for a name change in the case where the this parameter is not being changed (is already the bound method's parent). The interdiff -w output is easy to read. I'll attach that next. /be

Comment on attachment 182729 [details] [diff] [review] fixed version of last patch Fixed, but gdb is misleading me, and for some reason my tests are passing. jst helped a ton and we now see that this patch can't work. The idea is sound, but XPConnect (unlike LiveConnect) does *not* make bound methods for each wrapped native instance. We need a different approach in detail, but the same in general. In the mean time, I believe this is good for LiveConnect safety. I'll test that in another bug (and seek testing help, since FC3 gdb is pretty damn broken). /be

jst has taken XPCNativeWrapper and rewritten it in C++ with a backward-compatible API, but with extra smarts: it wraps deeply (lazily), and if the second argument is not a string, it is taken as a constructor with which to do an instanceof test (so you can make sure you're getting what you expect). After talking at length today, he and I agreed to move his C++ version into XPConnect. We intend to automate it as a wrapper around XPCWrappedNatives, when chrome is operating on content, with auto-unwrapping when flowing back into a content native method, and with identity/equality ops in JS working as before. More on that soon. /be

This isn't done yet, but it works fairly well. This replaces the current XPCNativeWrapper that's written in JS in a backwards compatible way. It does that by looking at the arguments the constructor gets and if all arguments following the first one are strings then it defaults to working like the current one, only it's all dynamic and doesn't care what the string arguments to the constructor are. This new XPCNativeWrapper implementation exposes the wrapped object through a "wrappedJSObject" property (same name that's used with double wrapping in XPConnect today). In addition to that this also does automatic wrapping and unwrapping for XPCNativeWrapper, that is, you can pass an XPCNativeWrapper to any XPCOM method that expects an interface pointer and we'll get the wrapped object and pass it in stead of the XPCNativeWrapper, and also when chrome is accessing content code this patch does automatic wrapping with XPCNativeWrapper. The part that's really lame in this patch is the detection code that figures out if chrome is accessing a content object so that it knows to wrap the result in an XPCNativeWrapper. Brendan's got some ideas on that, the code is in xpcconvert.cpp. The other part that's lame is that the implementation of this new XPCNativeWrapper is far from ideal. It doesn't use getters and setters, and for every wrapping operation if always creates a new wrapper when it could re-use existing ones. We'd need a hash from XPCWrappedNative to XPCNativeWrapper to make that work. Should be pretty easy to do. I'll be out of town (and out of reach, mostly) for the coming week, so this is pretty much where I leave this for others to look at and hopefully continue working on.

Oh, and I forgot to mention that I intentionally left a bunch of printf()'s in the code to possibly help show this work or not work.

The main point here is to get people trying the patch out. I think I persuaded dveditz to try it. It's working for me so far. /be

benjamin, can you attach that followup patch we crave to set xpcnativewrappers=yes for Firefox's five (or so) chrome URI prefixes? Thanks again, /be

My design notes on the patch: http://wiki.mozilla.org/User:Brendan -- someone suggest a place where this should live for good. /be

With the patch applied I crash if I open (suite) browser and then mail from the window->mail menu or via ctrl+2. Starting mail from the commandline works fine.

The other things missing from the first attempt, besides the crash fix, are: 1. Code to configure the five chrome packages in firefox with bsmedberg's fine new xpcnativewrappers=yes option. 2. A call or calls to JS_FlagSystemObject to mark chrome windows and their descendants as "system". Bed soon, more tomorrow. /be

Comment on attachment 183880 [details] [diff] [review] jst's + bsmedberg's + my patch to optimize the former and enable the latter >Index: browser/base/content/browser.js >+ // content.wrappedJSObject.defa...? Nix the comment? >@@ -4411,18 +4407,17 @@ nsContextMenu.prototype = { >- var wrapper = new XPCNativeWrapper(this.link, "href", "baseURI", >- "getAttributeNS()"); >+ var wrapper = this.link; Fix indent? >Index: chrome/src/nsChromeRegistry.cpp >+static PRBool >+CheckFlag(const nsSubstring& aFlag, const nsSubstring& aData, PRBool& aResult) Document what aResult is and what the return value is? And maybe what the function does? > nsChromeRegistry::ProcessManifestBuffer(char *buf, PRInt32 This only changes "content" packages, right? But can't all non-"skin" packages can execute script (looking at nsChromeRegistry::AllowScriptsForPackage here)? More precisely, would it make sense to set all non-"content" stuff in a package to unconditionally require wrapping? >+ xpc->WantXPCNativeWrappers(urlp.get()); If this fails, we should bail out or something. We don't want to be starting if things that expect to be security-checked are not. >Index: js/src/jsdbgapi.c >+JS_GetScriptedCallerFilenameFlags(JSContext *cx, JSStackFrame >+ if (!fp) >+ fp = cx->fp; fp can still be null here. >Index: js/src/jsdbgapi.h Could we document these methods somewhere? I know we don't do it in the JS headers usually; do we have any in-tree API docs on JS? >Index: js/src/xpconnect/src/xpcconvert.cpp >+ printf("Content accessed from chrome, wrapping wrapper in XPCNativeWrapper\n"); Let's stick #ifdef DEBUG_XPCNativeWrapper around these? This is as far as I got so far; more tomorrow.

after applying the patches firefox seems to run fine, dhtml tests pass, normal browsing. the dom abuse seems stopped at first sight, but it is still possible chrome to call content when a javascript object is directly passed to chrome - as in sidebar.getInterfaces(m) where "m" is a luser js object.

bug 289074 testcases attachment 179946 [details] and attachment 180189 [details] (both abuse navigator.preference) are not fixed by this patch. The other testcases appear to be fixed. At least two of my extensions (Web Developer 0.9.3 and ConQuery 1.5.4) are broken, and break the browser. Most of the window comes up, but bookmarks aren't loaded and various tabbrowser things like filling in the location bar and the security UI don't work. The __proto__ change to browser.js also needs to be made to tabbrowser.js and nsContextMenu.js. At startup I get an error about a redeclaration of XPCNativeWrapper. Is that expected since it's now implemented in xpconnect itself? "JavaScript error: chrome://global/content/XPCNativeWrapper.js, line 56: redeclaration of const XPCNativeWrapper"

Similar to the changes made to browser.js, I think we want this.

Apply on top of previous patches. The earlier nsChromeRegistry patch only reads the xpcnativewrappers from app-chrome.manifest, but doesn't put that notation there. You can't even hand-edit for testing because that file gets blown away every start (in a debug build). This bit adds contents.rdf processing to the chrome registry, and xpcNativeWrappers="true" to most of the Firefox content rdf-manifest files. More will have to be done to cover the suite. Adding this does not fix the navigator.preference exploit. I'm not awake enough to decide if that means this patch isn't working or if that's just a different vulnerability.

Please do not use "the rest of the chrome registry patch"... I have the *.manifest build automation in my tree and am testing now.

This patch is ready-to-land by itself; it registers all the ffox content packages using .manifests instead of contents.rdf and fixes a logic error in my last patch.

Please make sure to NOT check in the xpfe changes from the "get rid of more __proto__" patch. The chrome registry changes need to be ported to suite first; at the moment (as of last patches posted in this bug) suite is NOT doing auto-wrapping.

(In reply to comment #95) > at the moment ... suite is NOT doing auto-wrapping. It really ought to, eh? We don't want to ship 1.8b2 with the mfsa2005-41 holes anymore than the Deer Park alpha.

If we're actually doing a suite release based on Gecko 1.8b2 (which it's not clear to me that we are, given the current suite situation), then yes, we need to make similar chrome registry changes there. Neil, do you think you could do that? Or know someone who could?

fwiw, with comment 82 and comment 87 on winxp, the jsshell crashes on every test in the js library, firefox won't start with a specified profile via -P, and it appears to not be able to load chrome urls from the command line.

Comment on attachment 183880 [details] [diff] [review] jst's + bsmedberg's + my patch to optimize the former and enable the latter >Index: js/src/xpconnect/src/XPCNativeWrapper.cpp >+ReWrapIfDeepWrapper(JSContext *cx, JSObject *obj, jsval v, jsval >+ // Re-wrap non-primitiv values if this is a deep wrapper (deep "non-primitive" >+ if (!rvalWrapper) { >+ return ThrowException(NS_ERROR_UNEXPECTED, cx); NS_ERROR_OUT_OF_MEMORY seems to make more sense. >+XPC_NW_GetOrSetProperty(JSContext *cx, JSObject *obj, jsval id, >+ // Be paranoid, don't let people use this as another objects "object's" >+ printf("Mapping wrapper[%d] to wrapper.item(%d)\n", JSVAL_TO_INT(id), #ifdef DEBUG_XPCNativeWrapper >+ printf("Calling setter for %s\n", >+ ::JS_GetStringBytes(JSVAL_TO_STRING(id))); Same. >+ printf("Calling getter for %s\n", >+ ::JS_GetStringBytes(JSVAL_TO_STRING(id))); Same. >+XPC_NW_NewResolve(JSContext *cx, JSObject *obj, jsval id, uintN >+ // Be paranoid, don't let people use this as another objects "object's" Probably file a followup on the "XXX make sure this doesn't get collected" comment? >+ printf("Wrapping function object for for %s\n", >+ ::JS_GetStringBytes(JSVAL_TO_STRING(id))); DEBUG_XPCNativeWrapper >+ // member. This new functions parent will be the method to call from "function's" >+XPC_NW_Construct(JSContext *cx, JSObject *obj, uintN argc, jsval >+ printf("Wrapping already wrapped object\n"); DEBUG_.... >+ printf(" %s\n", ::JS_GetStringBytes(JSVAL_TO_STRING(argv[i]))); Same. >+XPC_NW_toString(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, >+ // Be paranoid, don't let people use this as another objects "object's" >+ resultString.Append(NS_REINTERPRET_CAST(jschar *, I'm pretty sure you need to cast to PRUnichar* here to avoid build bustage on some platforms... >+XPCNativeWrapper::AttachNewConstructorObject(XPCCallContext &ccx, >+ JSObject *class_obj = >+ ::JS_InitClass(ccx, aGlobalObject, nsnull, ... >+ NS_ASSERTION(class_obj, "Can't initialize XPCNW class."); That could be null on OOM or whatever. Change to warning, esp. since we bail out next line if it's null. With those utter nits picked and the chrome registry changes I asked for, sr=bzbarsky

Comment on attachment 183880 [details] [diff] [review] jst's + bsmedberg's + my patch to optimize the former and enable the latter >- reference = focusedWindow.__proto__.getSelection.call(focusedWindow); >+ reference = focusedWindow.getSelection.call(focusedWindow); Why not just reference = focusedWindow.getSelection(); ? >+ /* Objects always require "deep locking", i.e., rooting by value. */ >+ if (lock || type == GCX_OBJECT) { >+ if (lock == 0 || type != GCX_OBJECT) { So in this case, if I have a non-object which is already locked, and I lock it again, I will have |lock| but type != GCX_OBJECT. So I'll fall into this code, which nets out to: >+ if (!rt->gcLocksHash) { >+ rt->gcLocksHash = >+ JS_NewDHashTable(JS_DHashGetStubOps(), NULL, >+ sizeof(JSGCLockHashEntry), >+ GC_ROOTS_SIZE); >+ if (!rt->gcLocksHash) > goto error; > } else { >+#ifdef DEBUG >+ JSDHashEntryHdr *hdr = > JS_DHashTableOperate(rt->gcLocksHash, thing, > JS_DHASH_LOOKUP); >+ JS_ASSERT(JS_DHASH_ENTRY_IS_FREE(hdr)); >+#endif > } >+ lhe = (JSGCLockHashEntry *) >+ JS_DHashTableOperate(rt->gcLocksHash, thing, JS_DHASH_ADD); >+ if (!lhe) >+ goto error; >+ lhe->thing = thing; >+ lhe->count = 1; > } else { and end up with a count of 1 on the lhe. Then if we do a single Unlock on the same non-object, we'll hit a count of 0 and remove it from the hash table, when we should still have an entry for it. Or we'll blow the JS_DHASH_ENTRY_IS_FREE(hdr) assertion instead if the gcLocksHash is already allocated and we're locking a second time. I must be missing something here. Spell it out for me? > JSBool > js_UnlockGCThingRT(JSRuntime *rt, void *thing) > { >+ uint8 *flagp, flags; > JSGCLockHashEntry *lhe; > > if (!thing) > return JS_TRUE; > >+ flagp = js_GetGCThingFlags(thing); > JS_LOCK_GC(rt); >+ flags = *flagp; > >+ if (flags & GCF_LOCK) { >+ lhe = (JSGCLockHashEntry *) >+ JS_DHashTableOperate(rt->gcLocksHash, thing, JS_DHASH_LOOKUP); Are we guaranteed that gcLocksHash is allocated by this point? If we only did one lock on a non-object, we'll just have flagged it as GCF_LOCK, and never had to allocate the hash, I think. (Update: mconnor just crashed here, I think.) >+ * Try to inherit flags by prefix. We assume there won't be more than a >+ * few (dozen! ;-) prefixes, so linear search is tolerable. >+ * XXXbe every time I've assumed that in the JS engine, I've been wrong! I didn't mentally execute the code, so forgive me if this is a naive question, but: what keeps us from having a prefix entry per web-loaded script? Is it simply that we will tend to have one script prefix per loaded page, and trust our other limiting factors to keep us to a few dozen? Do we care about the DOS attack that comes from a bunch of <script src="data:a = <random>"></script> entries? (It's always been fun to watch you recover from those O(small-enough) assumptions, though!) >+/* void wantXPCNativeWrappers (in string filenamePrefix); */ >+NS_IMETHODIMP >+nsXPConnect::WantXPCNativeWrappers(const char *aFilenamePrefix) This function seems strangely named to me, in that we want wrappers when manipulating objects that _don't_ come from a system prefix, and Want here makes me think that we want wrappers "for" the provided prefix, rather than that the script running in this prefix wants (Expects?) to get wrappers when reaching out. The IDL doc comment doesn't help much either, since it doesn't really explain which side of the line we're "matching" for. nsXPConnect::addSystemPrefix ? >+ if (wrapper->GetScope() != xpcscope) >+ { >+ // Cross scope access detected. Check if chrome code >+ // is accessing non-chrome objects, and if so, wrap >+ // the XPCWrappedNative with a XPCNativeWrapper to >+ // prevent userdefined properties from shadowing DOM >+ // properties from chrome code. >+ >+ uintN flags = JS_GetScriptedCallerFilenameFlags(ccx, nsnull); >+ if((flags & JSFILENAME_SYSTEM) && >+ !JS_IsSystemObject(ccx, wrapper->GetFlatJSObject())) This looks nice, but I can't figure out where the system flag gets set on objects. I thought it might come from being created while a system-prefixed script was running, but I don't see that flag inheritance, and I don't see any calls to FlagSystemObject anywhere else. >+ printf("Calling setter for %s\n", >+ ::JS_GetStringBytes(JSVAL_TO_STRING(id))); >+ printf("Calling getter for %s\n", >+ ::JS_GetStringBytes(JSVAL_TO_STRING(id))); #ifdef DEBUG_xpcwrappednative or some such, pls. >+ // Be paranoid, don't let people use this as another objects "object's" >+ } else if (member->IsAttribute()) { >+ // An attribute is being resolved. Define the property, the value >+ // will be dealt with in the get/set hooks. >+ >+ // XXX: We should really just have getters and setters for >+ // properties and not do it the hard and expensive way. Agreed; is there a bug on file? >+ // XXX: make sure this doesn't get collected before it's hooked in >+ // someplace where it's kept around. Mmm, yes. File a bug? r-, I think the gcLockHash issues are probably real pain, and I'd like to understand the GCF_SYSTEM stuff better, even if it isn't really a bug.

Comment on attachment 183905 [details] [diff] [review] The rest of the chrome registry patch bsmedberg says not to use this one.

bz: I've fixed those nits, mostly jsts unique possessive unpunctuation style ;-). shaver: thanks, I was sleepy during that lock bit reclamation. But you then got sleepy too -- the prefix list is not one per script filename, but one per call to the new JS_FlagScriptFilenamePrefix API (see the wiki), and that is governed by chrome -- so there's no DOS-from-web-content hazard. Still, the list could get large if extensions go crazy, but that would be a signal to flip the default to xpcnativewrappers=yes. I'll have a better all-in-one patch shortly. /be

It turns out that attachment 183912 [details] [diff] [review] messes up Thunderbird more than a little bit because thunderbird repackages chrome in its own inimitable way and uses a hand-written installed-chrome.txt. I've been planning on getting rid of that chrome repackaging for a while, and I've finally done it. win32 installer continues to work correctly as well. Scott, you don't need to review the browser/* or chrome/* bits, just the mail/* and various xpfe/* changes which affect tbird.

(In reply to comment #97) >Neil, do you think you could do that? Or know someone who could? It wouldn't be easy to use with the provided xpconnect model. Toolkit converts any contents.rdf files to .manifest files; these are then parsed on every launch and any xpcnativewrapper flag is used to notify xpconnect. Suite on the other hand simply maintains two RDF data sources, one for the install data source and one for the current profile data source. It would have to manually scrape the data sources for xpcnativewrapper flags on every profile switch. Fortunately we can probably hack something in at the end of AddToCompositeDataSource(). Note that there's no way to turn the flag off, so if you switch to a profile with an installation of an extension that needs them turned off you're out of luck...

(In reply to comment #96) > (In reply to comment #95) > > at the moment ... suite is NOT doing auto-wrapping. > > It really ought to, eh? We don't want to ship 1.8b2 with the mfsa2005-41 holes > anymore than the Deer Park alpha. Who is "we"? Suite releases are being done by volunteers now, not by mozilla.org staff or MF employees. My brain hurts enough without having to worry about the suite, so I am not going to worry about it. It's someone else's turn... they're "it" :-/. /be

I give up on doing an all-in-one patch. I'll attach a patch to the infrastructure and let bsmedberg track the shaver-induced nsIXPConnect method name change, and attach a follow-on patch. I'm also not going to worry about rolling up dveditz' __proto__-policing patch, although I've applied it locally. /be

This does include bsmedberg's chrome code changes, but not the manifest changes all over the place (but I've got those in my tree, I think). I can't debug -- XPCOM autoreg runs *every* time and horks gdb badly. Anyone else see this re-registration bug? What's the bug # tracking it? Mainly this needs testing and debugging help. Boris is gonna do that, but more are welcome to join in. /be

Patch in comment 103 makes a suite build die with: error: file '../../../mozilla/toolkit/components/cookie/content/contents.rdf' doesn't exist at ../../../mozilla/config/make-jars.pl line 428, <STDIN> line 207. Given that some of our tinderbox tests only run on suite tinderboxen, we might want to not check that in as-is...

bsmedberg: bz and I both see XPCOM registration every single time, in our debug firefox builds. I also crash trying to run with -profileManager. No time to debug that. I use -P test (and my test profile has worked well for trunk builds in general), but when darin said what he was using, -profile <name>, I tried that and got WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file nsAppRunner.cpp, line 1325 followed by app exit. Is there a bug filed on any of this? /be

Firefox starts up fine, and this latest patch fixes the navigator.preference testcase again. Don't have extensions in this profile, will have to quit and try those separately.

Web Developer and ConQuery extensions still mess up browser chrome. Web Developer actually appears to work (haven't tried all features), but having it enabled seems to interrupt browser initialization. Bookmarks aren't loaded, securityUI isn't initialized, doesn't load the start page, etc. Menus and toolbar are there, though. The very first time I ran with this patch I got a PR_assert in the JS_AQUIRE_LOCK() in js_SaveScriptFilenameRT(), ultimately called from bsmedberg's new chrome stuff but everything looked kosher on his end. Haven't been able to reproduce that.

This deals with the zero-contexts-in-runtime condition bz saw during the double (!) XPCOM component registration madness that afflicts both his and my build at startup, every time. For him, this led to prefix losses that were not covered up by later chrome re-registration. Something about his .mozconfig differs from mine because I didn't note any prefix lossage. Anyway, this patch keeps script filename prefixes added by the new jsdbgapi.h entry point JS_FlagScriptFilenamePrefix around till runtime destruction. If we need to unload extensions, switch profiles, or otherwise unflag prefixes, we can add the obvious counterpart API. /be

OK, I tracked down the issue I was seeing. In SaveScriptFilename, when we're handling the |flags != 0| case, we screw up any time a filename that is strictly shorter than all already-inserted filenames is inserted, as long as it's the third or later one. Proof: sfp will get set to non-null any time head->next != head (so we have more than one thing in the list already). The only time it's set to null after this point is if |sfp->length <= length|, which will never happen if the new filename is shorter than all existing ones. So if we had > 1 filenames inserted and insert a new short one, we'll end up not actually adding it to the list and instead just munging the flags of whatever the last thing in the list was some more. I added an |sfp = NULL;| at the very end of the loop, and now I actually get XPCNativeWrappers created when I open the context menu. On a related note, it seems to me that if there is only one thing in the list we'll never enter the loop, so if someone adds it again we'll have two entries for it in the list.

(In reply to comment #113) > sfp will get set to non-null any time head->next != head (so we have more > than one thing in the list already). I pointed out that this is a list with one or more entries, not two or more. Then I realized bz and I were looking at a dump of the circular list where the list head (a JSCList) was being mis-cast to ScriptFilenamePrefix. So we were chasing a phantom. The real bug here, which we're hunting now, is that bz gets no prefix for chrome://browser/ -- but I do. /be

(In reply to comment #114) > The real bug here, which we're hunting now, is that bz gets no prefix Duh, bz pointed out the real bug -- terminating that for loop by reaching the end of the circular list (wrapping to the header) without nulling sfp. This patch fixes that bug, and optimizes/cleans-up XPCNativeWrapper.cpp slightly. Getting close. Interdiff of last patch and this one next. /be

For bc and others following the bouncing patch-ball. /be

assuming I patched correctly... js shell passes js tests, firefox asserts on startup. NTDLL! 7c901230() PR_Lock(PRLock * 0x00000000) line 236 + 28 bytes js_SaveScriptFilenameRT(JSRuntime * 0x0215a348, const char * 0x0012ee64, unsigned long 1) line 1115 + 16 bytes JS_FlagScriptFilenamePrefix(JSRuntime * 0x0215a348, const char * 0x0012ee64, unsigned long 1) line 1299 + 17 bytes nsXPConnect::FlagSystemFilenamePrefix(nsXPConnect * const 0x020f3690, const char * 0x0012ee64) line 1287 + 16 bytes nsChromeRegistry::ProcessManifestBuffer(char * 0x02150830, int 162, nsILocalFile * 0x020f25e8, int 0) line 1920 nsChromeRegistry::ProcessManifest(nsILocalFile * 0x020f25e8, int 0) line 1780 + 24 bytes nsChromeRegistry::CheckForNewChrome(nsChromeRegistry * const 0x020ef4c8) line 1141 + 19 bytes nsChromeRegistry::Init() line 541 nsChromeRegistryConstructor(nsISupports * 0x00000000, const nsID & {...}, void * * 0x0012f404) line 50 + 128 bytes nsGenericFactory::CreateInstance(nsGenericFactory * const 0x020ef480, nsISupports * 0x00000000, const nsID & {...}, void * * 0x0012f404) line 82 + 21 bytes nsComponentManagerImpl::CreateInstanceByContractID(nsComponentManagerImpl * const 0x020d1ac0, const char * 0x013cc9a4, nsISupports * 0x00000000, const nsID & {...}, void * * 0x0012f404) line 1987 + 24 bytes nsComponentManagerImpl::GetServiceByContractID(nsComponentManagerImpl * const 0x020d1ac4, const char * 0x013cc9a4, const nsID & {...}, void * * 0x0012f470) line 2414 + 50 bytes CallGetService(const char * 0x013cc9a4, const nsID & {...}, void * * 0x0012f470) line 95 nsGetServiceByContractID::operator()(const nsID & {...}, void * * 0x0012f470) line 278 + 19 bytes nsCOMPtr<nsIToolkitChromeRegistry>::assign_from_gs_contractid(nsGetServiceByContractID {...}, const nsID & {...}) line 1272 + 17 bytes nsCOMPtr<nsIToolkitChromeRegistry>::nsCOMPtr<nsIToolkitChromeRegistry>(nsGetServiceByContractID {...}) line 678 ScopedXPCOMStartup::SetWindowCreator(nsINativeAppSupport * 0x01aaf6f8) line 651 ProfileLockedDialog(nsILocalFile * 0x01aa7618, nsILocalFile * 0x01aa7618, nsIProfileUnlocker * 0x00000000, nsINativeAppSupport * 0x01aaf6f8, nsIProfileLock * * 0x0012fac8) line 1095 + 12 bytes SelectProfile(nsIProfileLock * * 0x0012fac8, nsINativeAppSupport * 0x01aaf6f8, int * 0x0012fab4) line 1472 + 40 bytes XRE_main(int 1, char * * 0x01aa7028, const nsXREAppData * 0x0123901c kAppData) line 1823 + 51 bytes main(int 1, char * * 0x01aa7028) line 61 + 18 bytes mainCRTStartup() line 338 + 17 bytes KERNEL32! 7c816d4f() in PR_Lock: + lock 0x00000000 me->flags 136 in js_SaveScriptFilenameRT: + filename 0x0012ee64 "chrome://browser/" flags 1 + rt 0x0215a348 rt->scriptFilenameTableLock 0x00000000 - sfe 0x0012ee64 - next 0x6f726863 next CXX0030: Error: expression cannot be evaluated keyHash CXX0030: Error: expression cannot be evaluated key CXX0030: Error: expression cannot be evaluated value CXX0030: Error: expression cannot be evaluated keyHash 792356205 key 0x6f72622f flags 1919251319 mark 47 '/' + filename 0x0012ee75 ""

FWIW, the flags are -P <name> or -profile <path>... and I'm pretty sure you have to create the <path> folder before you call -profile <path>.

(In reply to comment #117) > assuming I patched correctly... js shell passes js tests, firefox asserts on > startup. > > NTDLL! 7c901230() > PR_Lock(PRLock * 0x00000000) line 236 + 28 bytes > js_SaveScriptFilenameRT(JSRuntime * 0x0215a348, const char * 0x0012ee64, > unsigned long 1) line 1115 + 16 bytes Thanks, dveditz saw this too (bz and I do not). Perils of early init. Fixing for the next patch. Easy inline interdiff preview below. /be ----- cut here ----- diff -u js/src/jsscript.c js/src/jsscript.c --- js/src/jsscript.c 19 May 2005 05:56:38 -0000 +++ js/src/jsscript.c 19 May 2005 16:45:51 -0000 @@ -1112,11 +1112,16 @@ { ScriptFilenameEntry *sfe; + /* This may be called very early, via the jsdbgapi.h entry point. */ + if (!rt->scriptFilenameTable && !js_InitRuntimeScriptState(rt)) + return NULL; + JS_ACQUIRE_LOCK(rt->scriptFilenameTableLock); sfe = SaveScriptFilename(rt, filename, flags); JS_RELEASE_LOCK(rt->scriptFilenameTableLock); if (!sfe) return NULL; + return sfe->filename; } diff -u js/src/xpconnect/src/xpcprivate.h js/src/xpconnect/src/xpcprivate.h --- js/src/xpconnect/src/xpcprivate.h 19 May 2005 05:56:47 -0000 +++ js/src/xpconnect/src/xpcprivate.h 19 May 2005 16:46:01 -0000 @@ -139,7 +139,7 @@ #define DEBUG_xpc_hacker #endif -#if defined(DEBUG_brendan) || defined(DEBUG_jst) +#if defined(DEBUG_brendan) || defined(DEBUG_bzbarsky) || defined(DEBUG_jst) #define DEBUG_XPCNativeWrapper 1 #endif

I looked into the web developer extension issues. First off, the security UI issue is not caused by this patch. It's caused by bug 294815 and has been around on tip for a while now (since April 11). That exception is what breaks every single other thing listed as going wrong in comment 111 (as in, when I commented out the throwing line in browser.js I got bookmarks, start page, etc). The line in question is http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#3361 and since it only runs when the securityUI is null (due to bug 294815), it's just obviously wrong.... mconnor says we have a bug on that already somewhere, with patch even.

Brendan, the reason we saw wrapping with the webdeveloper extension is that it gets its documents like so: const documentList = webdeveloper_getDocuments(getBrowser(). browsers[mainTabBox.selectedIndex].contentWindow, new Array()); The thing is, getting the contentWindow property of a <xul:browser> goes through XBL, so ccx.mJSContext->fp->down->script->filename is "chrome://global/content/bindings/browser.xml" (and ccx.mJSContext->fp->script is null as usual). If I look at ccx.mJSContext->fp->down->down->script->filename then it's "chrome://webdeveloper/content/css.js" as expected, but that doesn't help us, of course. This is likely to bite other extensions too, I would bet, since getting the window for the "current tab" is pretty common. Perhaps the answer is to not set the "want wrappers" flag on chrome://global/ for now? What lives in that package anyway?

The issue we were seeing with webdeveloper.js failing on "headElementList[0] has no properties" is due to the following code in XPC_NW_GetOrSetProperty: if (!member->IsAttribute()) { // Getting the value of a method. Just return and let the value // from XPC_NW_NewResolve() be used. return JS_TRUE; } The problem is that in this case |member| is "item()", since |id| is an integer. And "item()" is a function, not an attribute. So we bailed. At the same time, XPC_NW_NewResolve expects us to handle the "|id| is an integer" case in this code. Changing this block to say: if (!member->IsAttribute() && methodName == id) { // Getting the value of a method. Just return and let the value // from XPC_NW_NewResolve() be used. Note that if methodName != id // then we fully expect that |member| is not an attribute and we need // to keep going and handle this case below. return JS_TRUE; } makes things happy over here.

> Perhaps the answer is to not set the "want wrappers" flag on > chrome://global/ for now? What lives in that package anyway? tabbrowser.xml and contentAreaUtils.js do, and both have had security issues in the past. ViewSource and PrintPreview are also, dunno if that's any kind of problem (if so it's probably currently a problem).

One other option if a lot of extensions break is to hack the contentWindow (and contentDocument?) props on these bindings (browser/tabbrowser) to return an unwrapped object. I just tried that, and it does work (give the unwrapped object to the webdeveloper extension), but even code that wants wrapping gets an unwrapped object (since we don't go back out of JS when returning here). So we'd need to audit all calling code for these two getters and use XPCWrappedNative manually for them in Mozilla code... Shouldn't be too bad, really, if we have to do this.

One interesting thing I ran into -- the context menu never sees the content wrappers for some reason, just chrome wrappers for the nodes. Not sure why, really. But in any case, when the context menu comes up this.target.ownerDocument is not an XPCNativeWrapper and isn't equal to the wrappedJSObject of getBrowser().contentWindow.document (which _is_ wrapped).

(In reply to comment #119) > > Thanks, dveditz saw this too (bz and I do not). Perils of early init. Fixing > for the next patch. Easy inline interdiff preview below. Ok, I start up without crashing now. On the first run with firefox -P Debug and set MOZ_NO_REMOTE=1 it says that -P is not recognized but appeared to select the profile anyway. On subsequent starts, the -P message does not appear.

So I talked with bz about the problem in comment 121 (.contentWindow etc being a wrapper when accessed by an extension). I wonder if making <browser>s have an idl-interface with the contentWindow and contentDocument as attributes would solve the problem. Then we should unwrap into a native object and then rewrap without the XPCNativeWrapper when extensions access it. The interface would still be implemented in xbl-js of course, we'd just add it to the 'implements' list.

(In reply to comment #120) > The line in question is > http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#3361 and > since it only runs when the securityUI is null (due to bug 294815), it's just > obviously wrong.... mconnor says we have a bug on that already somewhere, with > patch even. bug 292604

I thought about it some more, and I don't think the XBL-idl approach will work. The basic problem is that the call from extension JS into the XBL JS will just be a JS call. No XPConnect involved unless the XBL-bound thing is passed to a native method or something like that. Perhaps the simplest solution is to have an XPCNativeAutoUnwrapper which will take and XPCNativeWrapper and on every get/set check who's doing the resolving. If it's a "system" script, then get/set on the XPCNativeWrapper, if it's not then get/set on the wrappedJSObject. Then we make contentWindow and contentDocument getters on browser/tabbrowser (and nothing else, I think) return this unwrapper beastie. That should keep most things nicely for most XPCNativeWrapper users while fast while allowing extensions to access the unwrapped document/window (and thence the rest of the DOM). Does that sound reasonable? If so, I think I'll try implementing it; I'm having no other bright ideas.

(In reply to comment #128) > > mconnor says we have a bug on that already somewhere, with patch even. > bug 292604 I applied that patch and the securityUI exception went away but my symptoms didn't clear up. Still no bookmarks or start page if Web Developer is enabled. Separate error: Clicking on an error link from the Javascript console opens viewsource but does not take you to the line containing the error. You get the error "pre has no properties" from chrome://global/content/viewSource.js line 292

Will it really be a js-to-js call even if it's declared as an interface? Won't it just look like any other interface to XPConnect, which once we call it happen to call into a XPConnect-created vtable? Or are we 'clever' enough to notice when js calls a js-implemented interface on an XPCOM object and optimize that?

(In reply to comment #131) > Will it really be a js-to-js call even if it's declared as an interface? JS-to-JS calls do not involve natives (whether wrapped or double-wrapped by XPCNativeWrapper around the usual XPCWrappedNative [names suck, don't change them]). (In reply to comment #129) > Perhaps the simplest solution is to have an XPCNativeAutoUnwrapper which > will take and XPCNativeWrapper and on every get/set check who's doing the > resolving. If it's a "system" script, then get/set on the XPCNativeWrapper, > if it's not then get/set on the wrappedJSObject. Then we make contentWindow > and contentDocument getters on browser/tabbrowser (and nothing else, I think) > return this unwrapper beastie. That should keep most things nicely for most > XPCNativeWrapper users while fast while allowing extensions to access the > unwrapped document/window (and thence the rest of the DOM). Hmm, why wouldn't we make XPCNativeWrapper do this automagically, all the time? When it is called from "system" chrome, it does its thing, but when called from non-"system" chrome, it simply forwards get/set property calls and other hooks to its wrappedJSObject. /be

> Hmm, why wouldn't we make XPCNativeWrapper do this automagically, all the time? Depends on how expensive the "is caller system?" check is. I was trying to minimize the number of such checks, I guess. I agree that if this is not an issue, then it's simpler to just do this in XPCNativeWrapper.

(In reply to comment #130) > I applied that patch and the securityUI exception went away but my symptoms > didn't clear up. Hmm... odd... they cleared up for me when I did basically that... > Separate error: Clicking on an error link from the Javascript console This is fixed by the change described in comment 122.

This is looking good for b2/fx1.1a1. /be

Comment on attachment 184058 [details] [diff] [review] infrastructure patch, v5 >Index: browser/base/content/browser.js > function checkForDirectoryListing() >- content.defaultCharacterset = getMarkupDocumentViewer().defaultCharacterSet; >+ content.wrappedJSObject.defaultCharacterset = >+ getMarkupDocumentViewer().defaultCharacterSet; This change is good. The rest of the changes in this file shouldn't go in until we're flagging our UI as wanting wrappers. >Index: chrome/src/nsChromeRegistry.cpp I already made some comments on this part; I'd really like them to be addressed, but Brendan's not the one to do it, seems to me. Benjamin, could you fix that stuff up? >Index: js/src/xpconnect/src/xpcwrappednative.cpp Do we need to worry about updating the "system" flag when changing scopes? What about wrapper reparenting? Probably followup-bug fodder here. Other remaining followups: 1) Sort out whether we need to do something for contentDocument/Window. Need to test adblock and friends. 2) Sort out the scope thing with context menus I ran into (working on this). r=bzbarsky

I checked in just the js and dom changes, plus the gutting of the XPCNativeWrapper.js files. It's up to bsmedberg to land the rest, to make this patch actually auto-wrap content when accessed from app chrome. /be

Brief summary of problem: To find the right XPC scope to look for a wrapper in, we get the parent object and get the XPC scope from that. But in this case the parent got a XPCNativeWrapper stuck on it, so we were coming our with the wrong scope. So we created wrappers for the target node, etc all in the XPC scope of the chrome window instead of in the scope of the content window. All the patch does is fix up the "get the XPC scope from that" step to know about XPCNativeWrapper

One other comment on the chrome reg changes, in addition to my other ones. What is + entry->flags |= PackageEntry::XPCNATIVEWRAPPERS; actually doing? I'm not seeing that flag tested for anywhere...

Comment on attachment 184071 [details] [diff] [review] Same, but with right include. r/sr/a=me, thanks -- bz is a tough patch's best friend. /be

Comment on attachment 183946 [details] [diff] [review] Stop repackaging tbird chrome, and use the same flat manifests as the rest of the world uses [checked in] r=mscott for the mail, mailnews and toolkit changes. I'm doing this under duress as I think it will take a couple days at least to shake these changes out for Thunderbird which means I won't be able to release alpha one for 1.1 when Firefox is ready (if it is ready tomorrow). Also, make sure you tweak the installer to remove qute.jar and messenger.jar. Also, I'm assuming you'll back me up when I start adding jar.mn ifdefs to xpfe, editor and toolkit to stop packaging files that my repackaging work was avoiding for me. Once this lands I'll start going through the JAR files by hand to identify the new files that we are building with that we weren't before so e can take them back out. I"d like to do that before we land the patch but I understand the pressures to get this in for Firefox supercede that.

I wanted to say that if we need a day or three to get this right, that's ok -- the state of the trunk is such that we can't predict Firefox and Thunderbird alpha 1 will be on the same day, although I agree they should have about the same versions of common code. I also wanted to back mscott's position that we should support "minimal linking" in the sense that good compiled code linkers do, but for chrome packaging. We shouldn't require Thunderbird to pick up pieces it doesn't want. But if there is a "what's in the common code platform" issue underlying the surface conflict here, let's have that out in a separate venue. /be

Just a heads up, the balsa tinderbox memory figures went up significantly since this went in. (Though there are no major changes on brad) RLk:780B -> RLk:13.0KB Lk:308KB -> Lk:1.36MB MH:8.00MB -> MH:9.30MB

(In reply to comment #144) > Just a heads up, the balsa tinderbox memory figures went up significantly since > this went in. (Though there are no major changes on brad) > > RLk:780B -> RLk:13.0KB > Lk:308KB -> Lk:1.36MB > MH:8.00MB -> MH:9.30MB Ah, I noticed that too, and filed bug 294893 for it. Are you / is anyone sure that this checkin is causing it?

Comment on attachment 183946 [details] [diff] [review] Stop repackaging tbird chrome, and use the same flat manifests as the rest of the world uses [checked in] This is checked in with an additional fix of other-licenses/branding and with some additional comments to match bz's review.

This is probably already reviewed, I'm just looking for a sanity check. /be

Comment on attachment 184113 [details] [diff] [review] cleanup/fixup patch for various chrome files sr=bzbarsky, but I'm going to debug why this stuff broke things, exactly. All these changes should be no-ops except the first hunk.

Ah, the following line breaks: var searchStr = focusedWindow.__proto__.getSelection.call(focusedWindow); since there proto of the XPCNativeWrapper has no getSelection or anything on it. I guess that's more or less expected.

Two more issues that we found: 1) We can end up with an XPCWrappedNative as the __parent__ of an XPCNativeWrapper. This is wrong. I'll post a patch to fix. 2) The object principal of an XPCWrappedNative should probably be the same as the object principal of the underlying XPCNativeWrapper. More generally, what should the __parent__ linkage of the XPCWrappedNative be? Brendan said he'd think about this.

Brendan, I think this is much cleaner than fixing every single PreCreate method...

Comment on attachment 184115 [details] [diff] [review] Fix parenting r+sr+a=me with conforming brace and if( style. /be

(In reply to comment #151) > Two more issues that we found: > > 1) We can end up with an XPCWrappedNative as the __parent__ of an > XPCNativeWrapper. This is wrong. I'll post a patch to fix. It's easy to do, and here bz did indeed transpose XPCWrappedNative and XPCNativeWrapper in this sentence. Let's say it again, with shorter names: a wn should never have an nw as its parent. > 2) The object principal of an XPCWrappedNative should probably be the same as > the object principal of the underlying XPCNativeWrapper. More generally, > what should the __parent__ linkage of the XPCWrappedNative be? Brendan > said he'd think about this. I believe the nw linkage up the tree, via __parent__ (and parentNode, but that's automated for the deep nw case) should mirror the wn up-linkage. The "down" linkage is isomorphic already for the deep nw case, which is the main point of this automation layer. At the top of the __parent__-linked ancestor line is a nw wrapping a content window, and its object principal should be the same as the content window's. To make this work, we either (a) teach caps about XPCNativeWrappers; or (b) make XPCNativeWrapper objects have private nsISupports that can QI in the way that caps requires to find object principals. Thoughts on the last choice? /be

After building with these changes, I am no longer able to: 1) Open the account Manager 2) Open the Filter Dialog 3) Open the Junk Mail Dialog I'm not seeing any JS errors or console messages. Could it be related to the security fix? I'll keep digging.

I also removed some ThrowException calls that were redundant (when a fallible JS API entry point returns false or null, an exception has already been thrown, or an OOM error reported). /be

Comment on attachment 184129 [details] [diff] [review] XPCNativeWrapper.cpp patch to make deep wrapper __parent__ also deep >Index: XPCNativeWrapper.cpp >+ // JS_NewObject already thread (or reported OOM). s/thread/threw/ r+sr=bzbarsky

My patches at attachment 184113 [details] [diff] [review] and attachment 184129 [details] [diff] [review] are checked in. /be

Since the private of our XPCNativeWrapper's JSObject is an XPCWrappedNative anyway, we may as well flag ourselves as JSCLASS_PRIVATE_IS_NSISUPPORTS. That removes the need for the GetScopeOfObject() hack, fixes object principals, and likely some other places in the code that look for the natives for a JSObject.

Comment on attachment 184130 [details] [diff] [review] Object principal fixup BRILLIANT! (best Basil Fawlty voice ;-). /be

One other issue, the start page no longer loads in the message pane.

I'm also no longer able to switch folders in the folder pane. I keep getting a JS error saying "Component is not Available" when we call: GetMessagePaneFrame().location

Got as far as listing the URL prefixs that want wrappers. Also needs chrome:xpcNativeWrappers="true" added to a few contents.rdf files.

the original change to mail\base\jar.mn removed (accidentally?) three files that Thunderbird needs to run. I've added those back in and I also needed to register an additional chrome package to avoid errors complaining about not being able to find contextHelp.js. This fixes the problems with: 1) dialogs in thunderbird not opening. Still having problems with: 1) start page no longer loads in the message pane's iframe. 2) unable to switch folders, JS exception saying Component is not Available.

Neil, don't we need to flag xpfe/communicator as needing wrappers too? Also, I'd use NS_ARRAY_LENGTH() instead of sizeof() - 1.

(In reply to comment #166) > Also, I'd use NS_ARRAY_LENGTH() instead of sizeof() - 1. these two have different meanings...

Comment on attachment 184151 [details] [diff] [review] suite changes checked in including communicator sr=bzbarsky if the communicator package is also marked as wanting wrappers.

Comment on attachment 184151 [details] [diff] [review] suite changes checked in including communicator Gets this security stuff working in Suite builds. Local version of patch contains update for review comments.