1.13 KB, application/x-zip
1.20 KB, application/octet-stream
992 bytes, application/octet-stream
4.75 KB, patch
|Details | Diff | Splinter Review|
We have tried Referencing the JS file in the resources section of the XBL. but this had no effect
Since bug 834697, XBL now runs in a separate compartment with Xray vision to content JS. So if you want to access a function defined in the page, you'll need to do: var win = XPCNativeWrapper.unwrap(window); win.test(); Does that work?
Jorge, we recently make some changes to the way XBL works that may break addons injecting XBL into content. Can we make sure this is appropriately communicated?
Or could we enable the old XBL behavior for remote XUL? Since remote XUL sure needs plenty of XBL.
smaug suggested that we just disable XBL scopes for remote XUL. This isn't hard in theory, but the current ordering makes this a little bit ugly. We compute whether to use an XBL scope in the XPCWrappedNativeScope constructor, at which point we know the principal of the content we're working with, but I don't think there's a good way to check if we're dealing with a XUL document or not. We could probably just add an public function to flip this bit on a scope, and call that during XUL document initialization, which should presumably come before any XBL has been injected into the XBL scope (we could assert this as well). bz, thoughts?
(In reply to Bobby Holley (:bholley) from comment #3) > Jorge, we recently make some changes to the way XBL works that may break > addons injecting XBL into content. Can we make sure this is appropriately > communicated? I'll add it to my list for 21, thanks.
I don't think the current xul/xbl-enabling flags only work for "XUL Documents". IIRC it enables using XUL and XBL any way you want from origins that have been white-listed. So disabling XBL scopes for those origins should do the trick. Of course, I assume that will eventually mean that enabling xul/xbl for an origin means trusting it even more.
So, there are a few problems with disabling XBL scopes for certain types of content. First, once we no longer need the current dom.xbl_scope pref, there's a lot of machinery that we want to rip out, especially relating to SOWs. Keeping same-scope in-content XBL means that we either have to disable important security wrappers in those cases, or never remove a lot of nasty machinery. Either way, it's not a good situation. Second, our automation infrastructure actually enables remote XUL for XBL tests and such. So if we want our automation to test the configuration we ship, we'd have to add a _second_ preference, something like dom.this_is_automation_so_really_use_xbl_scopes. This is kind of gross. We could maybe compromise on (1) by continuing to use XBL scopes but automatically waiving Xray in those cases. This may or may not be transparent enough to buy us compat in the majority of cases, and we'd still need the gross second pref. As DOM owner, this is jst's call.
I'm a colleague of the original poster. We've tried the code in Comment #2 and it does work, however it appears from our tests that the "var win = XPCNativeWrapper.unwrap(window);" line needs to be in the local scope - i.e. needs to be present in every method and constructor that calls out to an external file. If I'm wrong about it needing to be in local scope, please can you suggest where we can put it to make it globally available to all the methods in our XBL. In our case we have 14k lines of XBL code. Looking at the most common four calls we make there are over 800 instances, and there's a long tail of lesser calls beyond that. That's a lot of editing, and a lot of extra bytes to send; clearly it would be much better for us if Remote XUL could fall back to the old behaviour. [As an aside, we would dearly love to move to HTML, but it's really the XBL rather than the XUL that's keeping us on Remote XUL. After a port to XBL2 was aborted when it failed to gain any traction, we're keeping an eye on Web Components but are unlikely to migrate until it sees some real adoption. IOW we don't want you to keep legacy code hanging around forever, but it would be nice to have something practical to migrate to before the old code gets excised.]
Yeah, I think forcing all the remote XUL/XBL to be changed is no go atm. If we had web components, the situation would be different since it would be better to move away from XBL anyway in that case.
> i.e. needs to be present in every method and constructor that calls out to an external > file. It doesn't have to be inside every method, as long as every method calls win.whatever() and the "win" is stored somewhere that every method sees... For example, you could store "win" on "this" and use it from there. But yeah, I understand the compat issues here. We need to do _something_. The question i what the something can be so that we can still kill off SOWs and the attendant performance impact on normal DOM operation.
(In reply to Boris Zbarsky (:bz) from comment #12) > But yeah, I understand the compat issues here. We need to do _something_. > The question i what the something can be so that we can still kill off SOWs > and the attendant performance impact on normal DOM operation. If we decide we need to do something, I think the best solution is to waive Xray on the sandbox and implement the double pref so that we continue testing what we ship by default. This will effectively mean that we won't be testing this behavior very much on our own, but maybe that's ok.
> the best solution is to waive Xray on the sandbox and implement the double pref That makes sense to me, yes.
Clearing jst's NEEDINFO since I'm writing up a patch for this.
Created attachment 718608 [details] [diff] [review] Part 1 - Move XBL scope tagging into the sandbox constructor. v1 We need to do this, because we'll need to know whether we're an XBL scope before setting up the sandboxPrototype. This is a bit cleaner anyway.
Created attachment 718609 [details] [diff] [review] Part 2 - Waive Xray for XBL scopes on XUL-whitelisted domains (except for automation). v1
This will need to be backported, so nominating for tracking.
Comment on attachment 718608 [details] [diff] [review] Part 1 - Move XBL scope tagging into the sandbox constructor. v1 r=me
Comment on attachment 718609 [details] [diff] [review] Part 2 - Waive Xray for XBL scopes on XUL-whitelisted domains (except for automation). v1 We don't need a transitive Xray waiver down in that last hunk? r=me modulo that.
(In reply to Boris Zbarsky (:bz) from comment #20) > We don't need a transitive Xray waiver down in that last hunk? As mentioned on IRC, I don't think we need one, because we're explicitly overriding the wrapping behavior for all objects between two such scopes. linux64 test run: https://tbpl.mozilla.org/?tree=Try&rev=1d7672e9c897 win32 build for testing by Darren: https://tbpl.mozilla.org/?tree=Try&rev=c63394f75cd6
Darren, can you give these builds a try and let me know if it fixes the problem? http://firstname.lastname@example.org/try-win32/
Looks like my automation patch didn't do the trick for reftests. Joel, can you tell me everywhere I need to stick this pref to make sure it runs for our automation? In particular, I want to put it everywhere that we enable remote XUL.
(In reply to Bobby Holley (:bholley) from comment #22) > Darren, can you give these builds a try and let me know if it fixes the > problem? > > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bobbyholley@gmail. > com-c63394f75cd6/try-win32/ Bobby, This build seems to address the immediate problem in the test case we created, however when using it to run our full application, I am seeing the following errors repeatedly (under firefox 20 we are not seeing any of these errors) TypeError: Value does not implement interface EventListener. NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMTreeWalker.nextNode] Error: Permission denied to access object NS_ERROR_XPC_SECURITY_MANAGER_VETO: Security Manager vetoed action arg 0 [nsIXULTemplateBuilder.addListener]
automation.py.in will set preferences for reftest and mochitest. For xpcshell you will need to add it to the specific tests you care about. For talos, you would need to add it to http://hg.mozilla.org/build/talos/file/880a267bfacc/talos/PerfConfigurator.py#l223
Darren, for that first one (about Value does not implement interface EventListener) are you implementing nsIDOMEventListener with XBL bindings by any chance? If not, there's a good chance it's a security check failure too..
(In reply to Boris Zbarsky (:bz) from comment #26) > Darren, for that first one (about Value does not implement interface > EventListener) are you implementing nsIDOMEventListener with XBL bindings by > any chance? If not, there's a good chance it's a security check failure > too.. If I understand the question correctly, then no we're not implementing nsIDOMEventListener ourselves. There are number of oElem.addEventListener() calls where oElem is variously an element in the XBL content or the bound element itself (i.e. "this" in the XBL). There are a few cases of window.addEventListener() as well, plus various removeEventListener() calls. If that doesn't answer your question, could you explain it in layman's terms.
That answers my question, yeah. Bobby, sounds like thisobj unwrapping in the binding code is failing when given whatever wrappers are now being created...
(In reply to Joel Maher (:jmaher) from comment #25) > automation.py.in will set preferences for reftest and mochitest. Hm, is this really true? When I apply the "Part 2" patch, and run reftests with the following, the pref value appears to be true (the default). TEST_PATH=layout/reftests/bugs/reftest.list make reftest When I add the pref to layout/tools/reftest/reftest-cmdline.js, the pref appears to be false (the override we want for automation). Any thoughts on what might be going on here?
thanks for double checking, I had overlooked something, and for reftest we only add a few prefs: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#60
So it turns out the waiving solution doesn't really work, because doing things like window.addEventListener(fun, ...) fail, because fun gets wrapped for the content compartment, and ends up in an opaque wrapper. bz and I talked on IRC, and decided to try bypassing the XBL scopes for non-system/remote XBL, and installing it directly into the content scope. I seem to have gotten this working locally. Pushing to try: linux64 unittest run: https://tbpl.mozilla.org/?tree=Try&rev=f5af89baa3d2 win32 builds for Darren: https://tbpl.mozilla.org/?tree=Try&rev=bda0fd66d8a0
Darren, if all goes well, and the linux64 run comes out green, can you try out the builds at http://email@example.com/try-win32/ ?
Despite the Linux build appearing red (from comment 31), i decided to give the new build (comment 32) a go anyway. This is still not functional with our application and we are seeing a different set of errors from those reported in comment 24. I can give more details about the errors we see if you think this will be benefical
(In reply to Darren Rutter from comment #33) > Despite the Linux build appearing red (from comment 31), i decided to give > the new build (comment 32) a go anyway. Yeah, looks that was just a fatal warning about constructor initialization ordering. I'll fix it but it should be irrelevant to the functionality of the patch. > This is still not functional with our application and we are seeing a > different set of errors from those reported in comment 24. > > I can give more details about the errors we see if you think this will be > benefical Please do!
The errors we are seeing now are far more vague than those reported previously and as such i am unsure how useful these will be. Error: Permission denied to access property 'toString' Error: Permission denied to access property 'message' uncaught exception: unknown (can't convert to string) They are the three errors we are seeing. It is also worth noting that our tabbox elements are no longer functioning, the tabs are click-able but the tabpanel associated with the tab is not changing. These are standard XUL tab and tabpanel elements, however they are included as children within an xbl widget. I will alter the test file which i attached when opening the bug to also test the tabboxes in an environment simpler than our application
Created attachment 721645 [details] A XUL tabs test for the latest test build you sent me The latest attachment displays the errors i have seen with tabbox's in the latest build of firefox which you sent me (v22). Note that this works fine in firefox version 20
The attached zip file seems to be missing widget.xbl and standard_code.js.
(In reply to Darren Rutter from comment #38) > The zip contains three files. > a XUL File, > a XBL File and, > a CSS File. I only see bindings.css and index.xul.
Created attachment 723389 [details] Xul Tabs Test Have added another .zip file to replace the one that was incomplete
There seemed to be a problem with uploading the zip and letting Bugzilla auto-detect the mimetype. We've now put up another copy with the mimetype manually set (although Bugzilla is displaying it as application/octet-stream although we set it to application/x-zip). This copy appears to download and decompress okay here, but if you still experience issues we've also put a copy of the zip file on our website: http://www.twofold-software.com/test/xul_test_tabs.zip
Created attachment 726195 [details] Tabs in remote XUL, test case Attached a simplified version of the previous test code. This one removes the XBL entirely, paring the test down to just a simple XUL tabbox. When loaded as a Remote XUL application even this simple case fails to work in Firefox >=21. It's not clear to me whether this issue (tabs not working in Remote XUL) has the same cause as the original report in this bug (scripts not working for XBL in Remote XUL) so please let me know if this should be filed as separate report.
Sorry for the delay here. I've been distracted with other stuff. So, the basic issue here is that remote XUL massively increases the quantity of XBL that needs to play nice with XBL scopes, because so much of XUL is implemented in XBL. So it means that we need to audit not only our in-content bindings, but pretty much any XBL binding ever, because some remote XUL app might pull it into content. The testcase in comment 43 demonstrates that there are likely to be incompatibilities (it appears that this.tabbox ends up undefined for some reason - I didn't dig too much deeper than that). Talking to Boris, I think the best solution is just to disable XBL scopes for remote XUL domains. Patch coming up.
Created attachment 727268 [details] [diff] [review] Disable XBL scopes for XUL-whitelisted domains. v1
Comment on attachment 727268 [details] [diff] [review] Disable XBL scopes for XUL-whitelisted domains. v1 r=me
This was backed out while investigating Windows debug build bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/0174ae8897f8
It appears the issue was a needs-clobber. Re-landed. https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ad6568024d https://hg.mozilla.org/integration/mozilla-inbound/rev/fddb99719062
Darren, I'm hoping this stuff is all fixed now on Nightly. Can you check today's build and see if you have any other issues?
Bobby, Have just tested this on the latest nightly build and everything appears to be fixed. Do you have a time frame when you expect this will be available for test on the Aurora branch, as the issue was also affecting that build
Comment on attachment 727268 [details] [diff] [review] Disable XBL scopes for XUL-whitelisted domains. v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): XBL scopes, bug 834697 User impact if declined: Remote XUL will generally be pretty broken without it. We don't technically support remote XUL, but it will piss a lot of people off. Testing completed (on m-c, etc.): On m-c, everything appears to work Risk to taking this patch (and alternatives if risky): Relatively low risk. Should only affect remote-XUL whitelisted domains, which are kind of unsupported anyway. String or UUID changes made by this patch: None This patch also needs to be landed simultaneously with a small refactoring in bug 843231 and a followup patch from bug 854019. I've got the whole stack ready to go in my local tree, so I'm going to flag those for approval as well. Given the dependencies and the rebasing involved, I'll handle the uplift.
Comment on attachment 727268 [details] [diff] [review] Disable XBL scopes for XUL-whitelisted domains. v1 In Bug 834697 we enabled XBL scopes which impacted Remote XUL. This patch disable's XBL scopes for XUL-whitelisted domains to avoid the broken user impact
(In reply to Darren Rutter from comment #52) > Bobby, Have just tested this on the latest nightly build and everything > appears to be fixed. > > Do you have a time frame when you expect this will be available for test on > the Aurora branch, as the issue was also affecting that build Hi Darren as soon as :bholley lands this patch on aurora, within the next 24 hours the Aurora branch should have the fix for you which you could use to verify.
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/413d5d8de3c8 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/762893c10abf