Over in bug 462113 I'm adding a scrubber for <video> controls to provide UI control of the playback position. The controls are an XBL binding in content. This has generally worked so far, but now with the scrubber I've added trying to click-and-drag the scrubber handle causes a error to be thrown, and my onchange handler isn't called... Eg, load http://www.double.co.nz/video_test/test5.html, drag the scrubber, and get: JavaScript error: , line 0: Permission denied for <http://www.double.co.nz> to call method XULElement.QueryInterface on <http://www.double.co.nz>. The scrubber is a <xul:scale>. I also sometimes get the same error when clicking the play/mute buttons, which are now <xul:button>s in trunk nightlies. Whatever fails with the buttons apparently isn't critical, though, as they seem to still work. mrbkap was kind enough to help debug this, and knows the precise problem. Basically, sounds like we're hitting some code in nsScriptSecurityManager.cpp that's preventing content from accessing anonymous content.

bkap - can you spin up a patch I can test with, to see if there are more problems that will need addressed?

So, I looked into this today in more depth and it's harder to fix than we originally thought. XBL gets the context that it wants not from the context stack, but from the document that the element is bound to. So pushing a null context doesn't help since it's ignored. I'll give this more thought.

Obviously this isn't a fix for the problem, but it bypasses the security check so I can see what else might be a problem. Unfortunately it still doesn't let the video scrubber work, filing a new bug now (to block bug 462113).

blocking1.9.1+

Making this P1 because it blocks a P1 bug

There isn't an easy fix here on the caps side. The question really comes down to why we are censoring native anonymous content at all, and we can't really fix that at this point. I wonder if it's possible to make the bindings be regular XBL anonymous content instead of native anonymous content to sidestep this for now and we can revisit this code later (at the very least to document why we do this censoring at all).

(In reply to comment #6) > The question really comes down to why we are censoring native anonymous content at all We don't want web pages to modify native anonymous content.

So, now that I've got the video controls working, I've backed out my hacky patch (attached here), to see what actually breaks. It looks like this is mostly just preventing keyboard control... The scrubber mostly works; it can be dragged with the mouse but pressing a key (up/down arrow) throws an error. Presumably the fix for bug 472090 and workaround for bug 473103 avoided most of the serious breakage? I think this is still something we want to fix for accessibility reasons, but it doesn't seem serious enough to require addressing before Beta 3 freezes.

demoting based on comment #8.

On windows the scrubber doesn't work at all with: Error: Permission denied for <http://www.double.co.nz> to call method XULElement.QueryInterface on <http://www.double.co.nz>.

Truth be said they do work, I guess because of the way the video element works though it's kinda hard to tell. Even with buffered video it takes about 5-10 seconds for the scrubber to update and move to that part of the video. Not this bug, though.

(In reply to comment #6) > I wonder if it's possible to make the bindings be regular XBL anonymous content > instead of native anonymous content to sidestep this for now and we can revisit > this code later (at the very least to document why we do this censoring at > all). We could easily extend nsIAnonmyousContentCreator::CreateAnonymousContent to tell the nsCSSFrameConstructor caller that the content should not be native-anonymous. I'm not sure of the implications of that, though.

The first implication is that clicking on the content will allow the web page to get a reference to it (from the click event). After that it would be able to perform various DOM modifications on it. For example, it could toggle its display to "none", after which the frame tree will be _very_ confused... I could have sworn we had existing bugs on the combination of XBL and native anonymous content, by the way. The native anonymous content is the bound element for the XBL in this case, right? Would it work to implement nsISecurityCheckedComponent in the binding and whitelist the (hopefully minimal) list of things we need to access?

(In reply to comment #13) > The native anonymous content is the bound element for the XBL in this case, > right? Would it work to implement nsISecurityCheckedComponent in the binding > and whitelist the (hopefully minimal) list of things we need to access? Unfortunately not. The code that censors native anonymous content runs after we've determined we're allowed to access the element and throws unconditionally.

Really? The hack in comment 3 is in code that runs before the nsISecurityCheckedComponent checks, and it just sets rv. We then return if rv is success, and press on to nsISecurityCheckedComponent if it's failure. That's the only place where we check for native anon content in CAPS, no?

Oops, sorry. I misremembered the control flow. Yeah, that would work. I'm going to throw this over to bz so I can concentrate on JS blockers.

OK. So a bit of digging shows that the <videocontrols> _already_ does what I suggested. It makes everything allAccess. In fact, if you set up a click handler and look at event.originalTarget when clicking on the controls, you can get back things like progressmeters, buttons, etc. That much also worked in Fx3, oddly enough. I thought we'd blocked even that... But in Fx3, but _doing_ anything with the anonymous content failed. In Fx3.1, you can do anything that's quickstubbed. In particular, .style.display is. Some things are not, like cssFloat, but you can set them on the <videocontrols> in this case anyway, because it allows you to get or set anything you want. We should probably spin off a separate bug on the fact that if we can get to the node at all from the event we more or less lose the protection we had due to quickstubs...

Filed bug 475864. Back to this bug, even if that bug gets fixed the <videocontrols> will be accessible to page script, since it already implements nsISecurityCheckedComponent. Is that acceptable? If it is, then we can probably just not set the native-anonymous bit on it, assuming that we still flag it as anonymous in some other way. That said, I'm still trying to figure out why this completely breaks the controls.

I assume we needed the security checked component on the <videocontrols> so its own XBL could access it, by the way?

OK, I just experimented some, and the slider behavior seems to be the same whether the QI error in question happens or not. Dragging it around responds very very slowly and crappily and gives really poor feedback (presumably bug 473107), but it responds. So is this bug basically about the cosmetic issue of the error showing up in the JS console?

The original filing of this bug was because the controls didn't seem to work at all. Either that was a due to a different problem, or I accidentally worked around it (comment 8) The current state of affairs is that keyboard controls don't work on the scrubber. EG, page up / page down after focusing the scrubber should make the it jump around, instead it just throws a security error. [Oddly, pressing the spacebar when the play button it paused will activate it. I guess that's implemented differently?] So, this isn't a terribly serious problem, but I'm not sure what this implies for accessibility. I'm also a bit nervous about not fully understanding what else might be broken/wonky as a result of this issue. [But maybe that's just me, and I get the feeling we're already in oddville as a result of using XBL in content.]

(In reply to comment #19) > I assume we needed the security checked component on the <videocontrols> so its > own XBL could access it, by the way? I mostly inherited it from the original work doublec did; I looked at removing it in bug 448909 comment 63 (at the time, things worked without it), but you suggested it stay in comment 66. (In reply to comment #18) > Back to this bug, even if that bug gets fixed the <videocontrols> will be > accessible to page script, since it already implements > nsISecurityCheckedComponent. Is that acceptable? Well... I don't think it's a critical problem, since the video controls are unprivileged. Content can already disable the video controls (in fact, it has to explicitly specify <video src="foo" controls> to get them in the first place), so I don't think malicious content playing UI tricks is a concern. It's a bit of a concern in the context that some folks are already interested in customizing the video controls, and if there's a kludgy way to do that from content it's probably inevitable that someone use that... Having Firefox updates break sites because they were relying on almost-hidden implementation details of the video controls would be unfortunate. I'd like for that to not happen, but I'd also like a pony.

I don't care much about people who customize the controls in hacky ways and break because of it. That said, maybe we should white-list the things we really use here (ideally tested with quickstubs disabled), instead of white-listing everything? Past that, I just want to make sure we're not exposing any security bugs. Ideally, bug 475864 will make that less likely. I'll look into the keyboard issue.

OK, so the problem is that the scale.xml binding, which implements the key control stuff as far as I can tell, touches all sorts of properties on the DOM node. We might be able to get away with quickstubbing those properties (none of nsIDOMXULElement is quickstubbed at the moment), but that would mean that we depend on whether quickstubs for native anonymous content stay (and hence on how bug 475864 is fixed). Maybe that's the way to go for now if we can write tests for this. We could add hacks like not flagging this content native anonymous, or adding another flag to allow content script to touch it, but all of those hacks would be dangerous security-wise. The only reasonable approach I see here (past getting xbl2, with its better scoping for the js, implemented) is to disallow all native anonymous access from content by never handing it to content code and then to remove that check in the security manager. I'm not really happy doing that last bit for 1.9.1, though; I'd prefer that the "never hand native anon content to content code" bit get more testing than that.

(In reply to comment #24) > I'm not really happy doing that last bit for 1.9.1, though; I'd prefer that the > "never hand native anon content to content code" bit get more testing than > that. I agree.

Justin, you want to try this and see what things look like to you? I'm still not sure whether this makes me happy, but at least we'd know whether it's an option.

This seems to work... Although first click of the play or mute button still triggers a "Permission denied for <url> to call method XULElement.QueryInterface" error, but afaict nothing is actually broken. I can add tests for this (moving the scrubber with the keyboard), if that's what you're looking for.

If you can write tests for this, that would be wonderful, yeah.

This is bz's patch plus an addition to the videocontrols test to use the keyboard. [doKey() is copied from password manager's test_basic_form_autocomplete.html]

Tests should be added for a video that is injected via document.write because at the moment the controls are totally broken for that. What's more, if you mouseover the video element you get the error endlessly...

Is there a bug filed on that? It probably needs a quite different fix from this bug...

Just filed bug 479253, couldn't find another.

Dolske, are you looking for review here?

Well, the core fix is bz's, I just rolled in some tests. If bz's happy with his change, then I guess it's ready for review. :)

Whether I'm happy with it depends on whether it still works after bug 475864 is fixed. I suspect it won't.

Is bug 481559 dup of this? I am not doing any keyboard action there.

Yes, sounds like a dup.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090501 Shiretoko/3.5b5pre Everything seems to be working now, no errors, no wonky behavior.

What's the situation here? Bug 475864 is fixed, what else do we need to do?

This should be fixed now that bug 475864 is fixed.

We should get in some tests here.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre Ubiquity/0.1.5