Closed Bug 503926 Opened 15 years ago Closed 15 years ago

Ignore QueryInterface exposed by non-chrome-privilege content

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking1.9.1 --- .8+
status1.9.1 --- .8-fixed

People

(Reporter: jruderman, Assigned: mrbkap)

References

Details

Attachments

(2 files, 1 obsolete file)

Can we make XPConnect ignore QueryInterface in content JS objects, as suggested by roc in bug 503699 comment 4?

This would be a general fix for bugs where QI is weird (bug 503644, bug 503697) or has side effects (bug 503699).    Programmers expect QI to have no side-effects apart from perhaps creating a tearoff.  It's hard to reason about code where QI can have arbitrary side effects.
Flags: blocking1.9.2?
instanceof will still work, I assume, which flattens interfaces. Does it rely on QI?
Nochum, that's unrelated, this is QI *implemented by* content script.
So, in that case content objects would trivially support all requested interfaces? Or trivially *not* support any interfaces? Either way, it's easy to do this. I do note that if we allow them to implement all interfaces, then any code touching a maybe-content-JS-C++-thingi will still have to be careful for misbehavior...
Ideally, XPConnect would behave as if content hadn't mucked with QI.  For example, document.body should be able to QI to nsIDOMNode but not nsIDOMSVGAnimationElement.  Is that significantly harder than claiming to support no interfaces?

Claiming to support all interfaces sounds scary, I don't know why we'd want to do that.
For XPCWrappedNatives, I'd be perfectly happy if QI did absolutely nothing magic.

The issue is vanilla JSObjects (XPCWrappedJS); those need to be able to QI to the interface they were created for (esp. nsIDOMEventListener).

I think content objects should claim support for as few interfaces as we can get away with...
Attached patch Proposed fix (obsolete) — Splinter Review
Before I check this in, I need to go through and make sure that we actually make all components' global objects STOBJ_IS_SYSTEM. Otherwise, this should be good to go.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #389035 - Flags: superreview?(jst)
Attachment #389035 - Flags: review?(bzbarsky)
Hmm.  Could this cause issues for cases when people load chrome documents in content docshells?  In those cases the global is not system, right? Not that they should be doing it, but.... I'm looking at about:config here, say.  :(
Note that I'm all in favor of causing problems for such (ab)users.  Maybe it'll finally make the tabbrowser folks get off their butts and fix things...
Attachment #389035 - Attachment is obsolete: true
Attachment #389035 - Flags: superreview?(jst)
Attachment #389035 - Flags: review?(bzbarsky)
Comment on attachment 389035 [details] [diff] [review]
Proposed fix

In that case, I'll call GetObjectPrincipal before bailing. Hopefully, that'll let chrome code run without any sort of bad perf hit and still let the case bz points out in comment 7 still work.
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
The chrome mochitests test both the STOBJ_IS_SYSTEM codepath and the codepath bz mentioned in comment 7.
Attachment #390127 - Flags: superreview?(jst)
Attachment #390127 - Flags: review?(bzbarsky)
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

r=me, but I'd rather the test explicitly opened both its windows, so we don't depend on this "chrome tests run in a content docshell" thing that I think is a bug...
Attachment #390127 - Flags: review?(bzbarsky) → review+
Note also that UniversalXPConnect code will still fail here; do we want to be testing for that instead of system principal?  Might not be worth worrying about.
(In reply to comment #11)
> r=me, but I'd rather the test explicitly opened both its windows, so we don't

Would you mind if the first test is an iframe instead of a window?

(In reply to comment #12)
> Note also that UniversalXPConnect code will still fail here; do we want to be
> testing for that instead of system principal?  Might not be worth worrying
> about.

There's no way to do that test given our current setup. I'm also not sure what it would mean: |new function() { enableprivilege("UniversalXPConnect") }|? |enableprivielege("UniversalXPConnect"); new Object()|? Either way, by the time this code runs, it's likely that the privilege isn't enabled any more.
> Would you mind if the first test is an iframe instead of a window?

With type="content"?  Not at all; sounds great.
Attachment #390127 - Flags: superreview?(jst) → superreview+
http://hg.mozilla.org/mozilla-central/rev/94fc37d58985 ... using the same file for both the dialog and the frame was a little ugly, but not too bad.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 506249
This is needed for 1.9.1 to fix blocker bug 504021.
status1.9.1: --- → ?
blocking1.9.1: --- → ?
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

This applies cleanly to 1.9.1 and passes mochitest locally.
Attachment #390127 - Flags: approval1.9.1.6?
blocking1.9.1: ? → .7+
Attachment #390127 - Flags: approval1.9.1.6? → approval1.9.1.7?
Comment on attachment 390127 [details] [diff] [review]
Updated to comments

Approved for 1.9.1.7, a=dveditz for release-drivers
Attachment #390127 - Flags: approval1.9.1.7? → approval1.9.1.7+
Whiteboard: [needs 1.9.1 landing]
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.18+
Flags: blocking1.9.0.18+ → blocking1.9.0.19?
Flags: blocking1.9.0.19? → blocking1.9.0.19-
Whiteboard: [needs 1.9.1 landing]
Depends on: 1002313
You need to log in before you can comment on or make changes to this bug.