844783 - Calling external javascript functions from XBL has stopped working

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Darren Rutter]

Attachment: xbl_external_JS.zip

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+

Comment 1

[Darren Rutter]

We have tried Referencing the JS file in the resources section of the XBL. but this had no effect

Comment 2

[Bobby Holley (:bholley)]

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?

Comment 3

[Bobby Holley (:bholley)]

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?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Or could we enable the old XBL behavior for remote XUL? Since remote XUL sure needs plenty of XBL.

Comment 5

[Bobby Holley (:bholley)]

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?

Comment 6

[Jorge Villalobos [:jorgev] (he/him)]

(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.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Mark C]

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 11

[Olli Pettay [:smaug][[e-mail]]]

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 12

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 13

[Bobby Holley (:bholley)]

(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 14

[Boris Zbarsky [:bzbarsky]]

> the best solution is to waive Xray on the sandbox and implement the double pref

That makes sense to me, yes.

Comment 15

[Bobby Holley (:bholley)]

Clearing jst's NEEDINFO since I'm writing up a patch for this.

Comment 16

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 2 - Waive Xray for XBL scopes on XUL-whitelisted domains (except for automation). v1

Comment 18

[Bobby Holley (:bholley)]

This will need to be backported, so nominating for tracking.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 718608 [details] [diff] [review]
Part 1 - Move XBL scope tagging into the sandbox constructor. v1

r=me

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Bobby Holley (:bholley)]

(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

Comment 22

[Bobby Holley (:bholley)]

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/[e-mail]-c63394f75cd6/try-win32/

Comment 23

[Bobby Holley (:bholley)]

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.

Comment 24

[Darren Rutter]

(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]

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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 27

[Mark C]

(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 28

[Boris Zbarsky [:bzbarsky]]

That answers my question, yeah.  Bobby, sounds like thisobj unwrapping in the binding code is failing when given whatever wrappers are now being created...

Comment 29

[Bobby Holley (:bholley)]

(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?

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 31

[Bobby Holley (:bholley)]

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

Comment 32

[Bobby Holley (:bholley)]

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/[e-mail]-bda0fd66d8a0/try-win32/ ?

Comment 33

[Darren Rutter]

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

Comment 34

[Bobby Holley (:bholley)]

(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!

Comment 35

[Darren Rutter]

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

Comment 36

[Darren Rutter]

Attachment: 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

Comment 37

[Bobby Holley (:bholley)]

The attached zip file seems to be missing widget.xbl and standard_code.js.

Comment 38

[Darren Rutter]

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.

Comment 39

[Bobby Holley (:bholley)]

(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.

Comment 40

[Darren Rutter]

Attachment: Xul Tabs Test

Have added another .zip file to replace the one that was incomplete

Comment 41

[Darren Rutter]

Attachment: Xul Tabs Test

Comment 42

[Mark C]

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 43

[Mark C]

Attachment: 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.

Comment 44

[Bobby Holley (:bholley)]

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.

Comment 45

[Bobby Holley (:bholley)]

Attachment: Disable XBL scopes for XUL-whitelisted domains. v1

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 727268 [details] [diff] [review]
Disable XBL scopes for XUL-whitelisted domains. v1

r=me

Comment 47

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b75fab8561b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/775abfe4876f

Comment 48

[Ryan VanderMeulen [:RyanVM]]

This was backed out while investigating Windows debug build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0174ae8897f8

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5ad6568024d
https://hg.mozilla.org/mozilla-central/rev/fddb99719062

Comment 51

[Bobby Holley (:bholley)]

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?

Comment 52

[Darren Rutter]

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 53

[Bobby Holley (:bholley)]

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 54

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=a34ef87bd895

Comment 55

[bhavana bajaj [:bajaj]]

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

Comment 56

[bhavana bajaj [:bajaj]]

(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.

Comment 57

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/413d5d8de3c8
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/762893c10abf