Closed
Bug 844783
Opened 12 years ago
Closed 12 years ago
Calling external javascript functions from XBL has stopped working
Categories
(Core :: XBL, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: darrenr, Assigned: bholley)
References
Details
(Whiteboard: [leave uplift for bholley])
Attachments
(4 files, 4 obsolete files)
(deleted),
application/x-zip
|
Details | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
application/octet-stream
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:22.0) Gecko/20130224 Firefox/22.0
Build ID: 20130224031053
Steps to reproduce:
Existing Remote XUL code that calls an external javascript function from within a xbl constructor or method returns error.
Test attached requires remote xul to be whitelisted using remote xul manager or similar.
Actual results:
Firebug logs the following error:
'ReferenceError: [[function]] is not defined' where [[function]] is the name of the called function.
Expected results:
Function should have been called. This worked up to version 20 but has broken in version 21.
With the attached test an alert should be fired from the XBL constructor and when each button is clicked. In practice the calls in the constructor and Method sections fail on firefox 21+
Reporter | ||
Comment 1•12 years ago
|
||
We have tried Referencing the JS file in the resources section of the XBL. but this had no effect
Assignee | ||
Updated•12 years ago
|
Attachment #717830 -
Attachment mime type: text/plain → application/x-zip
Assignee | ||
Comment 2•12 years ago
|
||
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?
Assignee | ||
Comment 3•12 years ago
|
||
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?
Flags: needinfo?(jorge)
Comment 4•12 years ago
|
||
Or could we enable the old XBL behavior for remote XUL? Since remote XUL sure needs plenty of XBL.
Assignee | ||
Comment 5•12 years ago
|
||
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?
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(bzbarsky)
Comment 6•12 years ago
|
||
(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.
Flags: needinfo?(jorge)
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.
Assignee | ||
Comment 8•12 years ago
|
||
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.
Flags: needinfo?(bzbarsky) → needinfo?(jst)
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.]
Comment 10•12 years ago
|
||
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.
Comment 11•12 years ago
|
||
> 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.
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Comment 13•12 years ago
|
||
> the best solution is to waive Xray on the sandbox and implement the double pref
That makes sense to me, yes.
Assignee | ||
Comment 14•12 years ago
|
||
Clearing jst's NEEDINFO since I'm writing up a patch for this.
Flags: needinfo?(jst)
Updated•12 years ago
|
Assignee: nobody → bobbyholley+bmo
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 15•12 years ago
|
||
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.
Attachment #718608 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #718609 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 17•12 years ago
|
||
This will need to be backported, so nominating for tracking.
tracking-firefox21:
--- → ?
Updated•12 years ago
|
status-firefox21:
--- → affected
Comment 18•12 years ago
|
||
Comment on attachment 718608 [details] [diff] [review]
Part 1 - Move XBL scope tagging into the sandbox constructor. v1
r=me
Attachment #718608 -
Flags: review?(bzbarsky) → review+
Comment 19•12 years ago
|
||
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.
Attachment #718609 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 20•12 years ago
|
||
(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
Assignee | ||
Comment 21•12 years ago
|
||
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/
Flags: needinfo?(darrenr)
Assignee | ||
Comment 22•12 years ago
|
||
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.
Flags: needinfo?(jmaher)
Reporter | ||
Comment 23•12 years ago
|
||
(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]
Flags: needinfo?(darrenr)
Comment 24•12 years ago
|
||
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
Flags: needinfo?(jmaher)
Comment 25•12 years ago
|
||
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..
Comment 26•12 years ago
|
||
(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.
Comment 27•12 years ago
|
||
That answers my question, yeah. Bobby, sounds like thisobj unwrapping in the binding code is failing when given whatever wrappers are now being created...
Assignee | ||
Comment 28•12 years ago
|
||
(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?
Flags: needinfo?(jmaher)
Comment 29•12 years ago
|
||
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
Flags: needinfo?(jmaher)
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 30•12 years ago
|
||
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
Assignee | ||
Comment 31•12 years ago
|
||
Darren, if all goes well, and the linux64 run comes out green, can you try out the builds at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bobbyholley@gmail.com-bda0fd66d8a0/try-win32/ ?
Reporter | ||
Comment 32•12 years ago
|
||
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
Assignee | ||
Comment 33•12 years ago
|
||
(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!
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(darrenr)
Reporter | ||
Comment 34•12 years ago
|
||
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
Flags: needinfo?(darrenr)
Reporter | ||
Comment 35•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #721645 -
Attachment mime type: application/octet-stream → application/x-zip
Assignee | ||
Comment 36•12 years ago
|
||
The attached zip file seems to be missing widget.xbl and standard_code.js.
Flags: needinfo?(darrenr)
Reporter | ||
Comment 37•12 years ago
|
||
standard_code.js is not required for this test as there is no javascript needed for the test case.
The zip contains three files.
a XUL File,
a XBL File and,
a CSS File.
These are all that is required for the errors i have encountered.
Flags: needinfo?(darrenr)
Assignee | ||
Comment 38•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(darrenr)
Reporter | ||
Comment 39•12 years ago
|
||
Have added another .zip file to replace the one that was incomplete
Attachment #721645 -
Attachment is obsolete: true
Flags: needinfo?(darrenr)
Reporter | ||
Comment 40•12 years ago
|
||
Attachment #723389 -
Attachment is obsolete: true
Comment 41•12 years ago
|
||
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
Comment 42•12 years ago
|
||
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.
Assignee | ||
Comment 43•12 years ago
|
||
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.
Assignee | ||
Comment 44•12 years ago
|
||
Attachment #718608 -
Attachment is obsolete: true
Attachment #718609 -
Attachment is obsolete: true
Attachment #727268 -
Flags: review?(bzbarsky)
Comment 45•12 years ago
|
||
Comment on attachment 727268 [details] [diff] [review]
Disable XBL scopes for XUL-whitelisted domains. v1
r=me
Attachment #727268 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 46•12 years ago
|
||
Comment 47•12 years ago
|
||
This was backed out while investigating Windows debug build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0174ae8897f8
Comment 48•12 years ago
|
||
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
Comment 49•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c5ad6568024d
https://hg.mozilla.org/mozilla-central/rev/fddb99719062
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Assignee | ||
Comment 50•12 years ago
|
||
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?
Flags: needinfo?(darrenr)
Reporter | ||
Comment 51•12 years ago
|
||
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
Flags: needinfo?(darrenr)
Assignee | ||
Comment 52•12 years ago
|
||
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.
Attachment #727268 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave uplift for bholley]
Assignee | ||
Comment 53•12 years ago
|
||
Comment 54•12 years ago
|
||
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
Attachment #727268 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 55•12 years ago
|
||
(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.
Assignee | ||
Comment 56•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•