820666 - nsContentUtils::IsCallerXBL misses <field>s

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

The semantics of xbl <field>s are a bit wacky, but they can nonetheless define functions, and currently when those get called we return the wrong answer for IsCallerXBL. Patch coming up.

Comment 1

[Alex Keybl [:akeybl]]

Can you go into specifics about what the user impact would be here? I know this was called out specifically in relation to bug 810082.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Alex Keybl [:akeybl] from comment #1)
> Can you go into specifics about what the user impact would be here? I know
> this was called out specifically in relation to bug 810082.

I added nsContentUtils::IsCallerXBL in bug 795275, so that the Components warning would not appear for XBL callers, where we're allowing Components access. However, someone went and backported that patch for bug 810082 and used it there. I don't have a great grasp of what that patch is trying to do, so I can't properly evaluate the risks of getting it wrong. John probably knows though. John, if IsCallerXBL sometimes returns false for XBL callers, is that a problem?

The other impact of course is that we might issue spurious warnings to the console for legit XBL Components access. But it might be kind of rare. Then again, this patch is extremely low risk. Let's see what John says.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Tag XBL script for <field> elements and child scripts. v1

Attaching a patch. Hopefully Waldo can tell me if there's any other way XBL code
can propagate to new scripts where we miss propagating userBit. Note that I
explictly _don't_ want to do it for Eval. ;-)

Comment 4

[John Schoenick [:johns]]

(In reply to Bobby Holley (:bholley) from comment #2)
> I added nsContentUtils::IsCallerXBL in bug 795275, so that the Components
> warning would not appear for XBL callers, where we're allowing Components
> access. However, someone went and backported that patch for bug 810082 and
> used it there. I don't have a great grasp of what that patch is trying to
> do, so I can't properly evaluate the risks of getting it wrong. John
> probably knows though. John, if IsCallerXBL sometimes returns false for XBL
> callers, is that a problem?

In bug 810082 we're trying to respond to an event when content JS touches a plugin tag that is in CTP mode. Such tags have an XBL element attached to them, which has a bit of JS in its constructor. If that XBL tag being attached triggers the event, then we would essentially always think content was trying to touch the tag.

A quick test shows that this particular XBL does not erroneously trigger the event, does that mean we safe from this bug or might it still trigger in some circumstances?

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 691519 [details] [diff] [review]
Tag XBL script for <field> elements and child scripts. v1

Jorendorff and I have been talking about this, so I'm going to pass this to him.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 691519 [details] [diff] [review]
Tag XBL script for <field> elements and child scripts. v1

Review of attachment 691519 [details] [diff] [review]:
-----------------------------------------------------------------

Looks perfect.

Comment 7

[Bobby Holley (:bholley)]

Johns pointed me to pluginProblem.xml, which I verified doesn't contain <field>. So that bug should be unaffected.

Comment 9

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46826841e0fe

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/46826841e0fe