Closed Bug 281988 Opened 16 years ago Closed 16 years ago

Stop sharing DOM object wrappers between content and chrome

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: jst, Assigned: brendan)

References

Details

(Whiteboard: extension buster? needed for b2)

Attachments

(22 files, 14 obsolete files)

4.63 KB, patch
Details | Diff | Splinter Review
6.06 KB, patch
Details | Diff | Splinter Review
20.97 KB, patch
shaver
: review+
caillon
: review+
brendan
: approval1.8b2+
Details | Diff | Splinter Review
41.19 KB, image/png
Details
3.22 KB, patch
Details | Diff | Splinter Review
12.68 KB, patch
Details | Diff | Splinter Review
2.08 KB, patch
Details | Diff | Splinter Review
68.66 KB, patch
Details | Diff | Splinter Review
4.13 KB, text/plain
Details
4.33 KB, patch
Details | Diff | Splinter Review
102.74 KB, patch
mscott
: review+
Details | Diff | Splinter Review
7.92 KB, patch
Details | Diff | Splinter Review
129.42 KB, patch
bzbarsky
: review+
brendan
: approval1.8b2+
Details | Diff | Splinter Review
3.81 KB, patch
brendan
: review+
brendan
: superreview+
brendan
: approval1.8b2+
Details | Diff | Splinter Review
5.17 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
33.92 KB, patch
benjamin
: review+
bzbarsky
: superreview+
brendan
: approval1.8b2+
Details | Diff | Splinter Review
1.78 KB, patch
brendan
: review+
brendan
: superreview+
brendan
: approval1.8b2+
Details | Diff | Splinter Review
9.31 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
brendan
: approval1.8b2+
Details | Diff | Splinter Review
2.62 KB, patch
brendan
: review+
brendan
: superreview+
brendan
: approval1.8b2+
Details | Diff | Splinter Review
1020 bytes, patch
Details | Diff | Splinter Review
8.19 KB, patch
benjamin
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
633 bytes, patch
Details | Diff | Splinter Review
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.
Attachment #174106 - Attachment is obsolete: true
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
Flags: blocking1.8b2+
Flags: blocking-aviary1.1+
Blocks: 289187
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 :-(
Blocks: 288722
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?
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.
Blocks: 289074
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
Blocks: 289083
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...
Assignee: jst → brendan
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Blocks: 290908
Blocks: 290324
Whiteboard: extension buster? needed for b2
Attached patch closer, still needs some fixin' (obsolete) — Splinter Review
And testing.  Please help.

jst, I see that nsGlobalWindow::OnFinalize assertion.  Do I need your Aviary
version, or some part of it?

/be
Attachment #181807 - Attachment is obsolete: true
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
Attachment #182036 - Attachment is obsolete: true
Attachment #182537 - Flags: superreview?(jst)
Attachment #182537 - Flags: review?(shaver)
Attachment #182537 - Flags: approval1.8b2+
Attachment #182537 - Attachment description: no shared wrappers, but general native function security (prerequisite to any future patch) → no unshared wrappers, but general native function security (prerequisite to any future patch)
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
Attachment #182537 - Flags: review+
Comment on attachment 182537 [details] [diff] [review]
no unshared wrappers, but general native function security (prerequisite to any future patch)

sr=jst
Attachment #182537 - Flags: superreview?(jst) → superreview+
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.
Attachment #182537 - Flags: review?(shaver) → review+
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
Blocks: 292856
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.
Depends on: 292902
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
Attachment #182713 - Flags: superreview?(shaver)
Attachment #182713 - Flags: review?(caillon)
Attachment #182713 - Flags: approval1.8b2+
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
Attachment #182713 - Attachment is obsolete: true
Attachment #182713 - Flags: superreview?(shaver)
Attachment #182713 - Flags: review?(caillon)
Attachment #182713 - Flags: approval1.8b2+
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
Attachment #182729 - Flags: superreview?(jst)
Attachment #182729 - Flags: review?(caillon)
Blocks: 291314
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
Attachment #182729 - Flags: superreview?(jst)
Attachment #182729 - Flags: review?(caillon)
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.
Attachment #183856 - Flags: superreview?(brendan)
Attachment #183856 - Flags: review?(darin)
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
Attachment #183880 - Flags: superreview?(bzbarsky)
Attachment #183880 - Flags: review?(shaver)
Attachment #183880 - Flags: approval1.8b2?
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.
Attachment #183856 - Attachment is obsolete: true
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
Attachment #183880 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #183880 - Flags: superreview?(bzbarsky)
Attachment #183880 - Flags: superreview+
Attachment #183880 - Flags: review?(shaver)
Attachment #183880 - Flags: review-
Attachment #183880 - Flags: superreview?(bzbarsky)
Comment on attachment 183905 [details] [diff] [review]
The rest of the chrome registry patch

bsmedberg says not to use this one.
Attachment #183905 - Attachment is obsolete: true
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.
Attachment #183912 - Attachment is obsolete: true
Attachment #183946 - Flags: review?(mscott)
(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
Attached patch latest and greatest (obsolete) — Splinter Review
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
Attachment #183880 - Attachment is obsolete: true
Attachment #183887 - Attachment is obsolete: true
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.
Depends on: 294740
Attached patch infrastructure patch, v3 (obsolete) — Splinter Review
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
Attachment #183962 - Attachment is obsolete: true
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
Attached patch infrastructure patch, v4 (obsolete) — Splinter Review
(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
Attachment #183978 - Attachment is obsolete: true
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>.
Attachment #183880 - Flags: approval1.8b2?
Attachment #183856 - Flags: superreview?(brendan)
Attachment #183856 - Flags: review?(darin)
(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
Attachment #183982 - Attachment is obsolete: true
Attachment #184058 - Flags: review?(bzbarsky)
Attachment #184058 - Flags: approval1.8b2+
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
Attachment #184058 - Flags: review?(bzbarsky) → review+
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
Attachment #184070 - Flags: superreview?(brendan)
Attachment #184070 - Flags: review?(brendan)
Attachment #184070 - Attachment is obsolete: true
Attachment #184071 - Flags: superreview?(brendan)
Attachment #184071 - Flags: review?(brendan)
Attachment #184070 - Flags: superreview?(brendan)
Attachment #184070 - Flags: review?(brendan)
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
Attachment #184071 - Flags: superreview?(brendan)
Attachment #184071 - Flags: superreview+
Attachment #184071 - Flags: review?(brendan)
Attachment #184071 - Flags: review+
Attachment #184071 - Flags: approval1.8b2+
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.
Attachment #183946 - Flags: review?(mscott) → review+
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.
Attachment #183946 - Attachment description: Stop repackaging tbird chrome, and use the same flat manifests as the rest of the world uses. → Stop repackaging tbird chrome, and use the same flat manifests as the rest of the world uses [checked in]
Blocks: 294893
Attachment #184112 - Flags: superreview?(bzbarsky)
Attachment #184112 - Flags: review?(bzbarsky)
Attachment #184112 - Flags: approval1.8b2-
Attachment #184112 - Flags: approval1.8b2- → approval1.8b2?
This is probably already reviewed, I'm just looking for a sanity check.

/be
Attachment #184113 - Flags: superreview?(bzbarsky)
Attachment #184113 - Flags: review?(benjamin)
Attachment #184113 - Flags: approval1.8b2+
Attachment #184113 - Flags: review?(benjamin) → review+
Attachment #184112 - Flags: superreview?(bzbarsky)
Attachment #184112 - Flags: superreview+
Attachment #184112 - Flags: review?(bzbarsky)
Attachment #184112 - Flags: review+
Blocks: 294938
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.
Attachment #184113 - Flags: superreview?(bzbarsky) → superreview+
Attachment #184112 - Attachment description: Move xmlprettyprint to the "global" package, rev. 1 → Move xmlprettyprint to the "global" package, rev. 1 [checked in]
Attachment #184112 - Flags: approval1.8b2?
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.
Attached patch Fix parentingSplinter Review
Brendan, I think this is much cleaner than fixing every single PreCreate
method...
Attachment #184115 - Flags: superreview?(brendan)
Attachment #184115 - Flags: review?(brendan)
Comment on attachment 184115 [details] [diff] [review]
Fix parenting

r+sr+a=me with conforming brace and if( style.

/be
Attachment #184115 - Flags: superreview?(brendan)
Attachment #184115 - Flags: superreview+
Attachment #184115 - Flags: review?(brendan)
Attachment #184115 - Flags: review+
Attachment #184115 - Flags: approval1.8b2+
(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
Attachment #184129 - Flags: superreview?(bzbarsky)
Attachment #184129 - Flags: review?(bzbarsky)
Attachment #184129 - Flags: approval1.8b2+
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
Attachment #184129 - Flags: superreview?(bzbarsky)
Attachment #184129 - Flags: superreview+
Attachment #184129 - Flags: review?(bzbarsky)
Attachment #184129 - Flags: review+
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.
Attachment #184130 - Flags: superreview?(brendan)
Attachment #184130 - Flags: review?(brendan)
Comment on attachment 184130 [details] [diff] [review]
Object principal fixup

BRILLIANT! (best Basil Fawlty voice ;-).

/be
Attachment #184130 - Flags: superreview?(brendan)
Attachment #184130 - Flags: superreview+
Attachment #184130 - Flags: review?(brendan)
Attachment #184130 - Flags: review+
Attachment #184130 - Flags: approval1.8b2+
Depends on: 294968
One other issue, the start page no longer loads in the message pane.
No longer depends on: 294968
Depends on: 294968
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.
Depends on: 294983
Attachment #184139 - Attachment is obsolete: true
Attachment #184151 - Flags: superreview?(brendan)
Attachment #184151 - Flags: review?(benjamin)
Neil, don't we need to flag xpfe/communicator as needing wrappers too?

Also, I'd use NS_ARRAY_LENGTH() instead of sizeof() - 1.
Depends on: 295040
Attachment #184151 - Flags: review?(benjamin) → review+
Depends on: 295090
Depends on: 295101
Depends on: 295115
Depends on: 295122
Depends on: 295121
(In reply to comment #166)
> Also, I'd use NS_ARRAY_LENGTH() instead of sizeof() - 1.

these two have different meanings...
No longer depends on: 295115
Depends on: 295151
Depends on: 295152
Depends on: 295153
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.
Attachment #184151 - Flags: superreview?(brendan) → superreview+
Depends on: 295160
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.
Attachment #184151 - Flags: approval1.8b2?
No longer depends on: 295121
Attachment #184151 - Flags: approval1.8b2? → approval1.8b2+
Attachment #184151 - Attachment description: suite changes → suite changes checked in including communicator
Depends on: 295200
Sorry Neil, but your last Checkin for this Bug has caused another regression. 

When I try to compose a Message (Posting in Newsgroup) Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050523 Mnenhy/0.7.2.10001 {Build ID:
2005052302} crashes imediatly and reproduceable almost for me.

Mozilla Build 2005052221 without this patch works fine.
(In reply to comment #170)
>Sorry Neil, but your last Checkin for this Bug has caused another regression. 
Nah, it just detected a previously existing problem (filed as bug 295200).

If you want a workaround, then exit Mozilla, edit dist/bin/chrome/chrome.rdf and
add a line chrome:xpcNativeWrappers="true" under urn:mozilla:package:editor
With a clean clobber build from this morning, I am still unable to do the following:

1) Switch mail folders (make sure you've loaded a message then try to switch)
2) the mail start page still won't load in the message pane's browser element
anymore.

(In reply to comment #172)
> With a clean clobber build from this morning, I am still unable to do the
following:
> 
> 1) Switch mail folders (make sure you've loaded a message then try to switch)
> 2) the mail start page still won't load in the message pane's browser element
> anymore.
> 
> 

I am seeing the same thing. It works if you click back and forth to another
folder a couple of times. That, or click on another message, then back to the
folder you want.
Scott, I don't see a bug on the start page issue.  Please file one, and file
bugs for any other issues you run into?  This bug is far too unwieldy to put
more patches or discussion here...
Depends on: 295266
Flags: blocking1.8b2+
Depends on: 295301
(In reply to comment #172)
> With a clean clobber build from this morning, I am still unable to do the
following:
> 
> 1) Switch mail folders (make sure you've loaded a message then try to switch)

I filed a separate bug 295222 before I found this discussion.  I suppose it
could be marked a duplicate.
Depends on: 295520
Blocks: 295582
Depends on: 295782
Depends on: 295937
Trying to wade through all my backed up bugmail. So is this bug fixed and much 
of this just fall out that should have been filed as separate bugs, or is this 
issue still being addressed?
The last couple of patches here were just fallout, yes.  This bug is fixed.  So
are all the regressions we know about.  ;)
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
But we still track dependencies such as bug 295937 here.  Good way to be sure
all followup-bugs are fixed.

/be
Blocks: 290872
Depends on: 296902
Depends on: 296965
Depends on: 296967
Please remove the following entries from allmakefiles. 

http://lxr.mozilla.org/mozilla/source/allmakefiles.sh#1039

configure log message:

creating toolkit/components/passwordmgr/resources/content/contents.rdf
sed: ./toolkit/components/passwordmgr/resources/content/contents.rdf.in: No such
file or directory
creating toolkit/content/buildconfig.html
creating toolkit/components/passwordmgr/resources/content/contents.rdf
sed: ./toolkit/components/passwordmgr/resources/content/contents.rdf.in: No such
file or directory
creating gfx/gfx-config.h
Comment 179 seems to be unrelated to this bug... crot0@infoseek.jp, did you get
the wrong bug number?
Oh, that part... Please file a separate bug on that, make it block this one,
assign it to Benjamin.
Blocks: 299911
Depends on: 300325
Depends on: 300562
Depends on: 301476
Depends on: 301498
Depends on: 304886
Depends on: 307294
Blocks: 311451
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.