Calling external javascript functions from XBL has stopped working

RESOLVED FIXED in Firefox 21

Status

()

Core
XBL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Darren Rutter, Assigned: bholley)

Tracking

21 Branch
mozilla22
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox21+ fixed)

Details

(Whiteboard: [leave uplift for bholley])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 717830 [details]
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+
(Reporter)

Comment 1

4 years ago
We have tried Referencing the JS file in the resources section of the XBL. but this had no effect
Attachment #717830 - Attachment mime type: text/plain → application/x-zip
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?
Flags: needinfo?(jorge)

Comment 4

4 years ago
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?
Flags: needinfo?(bzbarsky)
(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.
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)

Comment 10

4 years ago
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.
Flags: needinfo?(jst)
Assignee: nobody → bobbyholley+bmo
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Attachment #718608 - Flags: review?(bzbarsky)
Created attachment 718609 [details] [diff] [review]
Part 2 - Waive Xray for XBL scopes on XUL-whitelisted domains (except for automation). v1
Attachment #718609 - Flags: review?(bzbarsky)
This will need to be backported, so nominating for tracking.
tracking-firefox21: --- → ?

Updated

4 years ago
status-firefox21: --- → affected
tracking-firefox21: ? → +
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 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+
(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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bobbyholley@gmail.com-c63394f75cd6/try-win32/
Flags: needinfo?(darrenr)
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 24

4 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)
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)
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

4 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.
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?
Flags: needinfo?(jmaher)
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)
Depends on: 834697
Blocks: 834697
No longer depends on: 834697
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://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bobbyholley@gmail.com-bda0fd66d8a0/try-win32/ ?
(Reporter)

Comment 33

4 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
(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!
Flags: needinfo?(darrenr)
(Reporter)

Comment 35

4 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 36

4 years ago
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
Attachment #721645 - Attachment mime type: application/octet-stream → application/x-zip
The attached zip file seems to be missing widget.xbl and standard_code.js.
Flags: needinfo?(darrenr)
(Reporter)

Comment 38

4 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)
(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.
Flags: needinfo?(darrenr)
(Reporter)

Comment 40

4 years ago
Created attachment 723389 [details]
Xul Tabs Test

Have added another .zip file to replace the one that was incomplete
Attachment #721645 - Attachment is obsolete: true
Flags: needinfo?(darrenr)
(Reporter)

Comment 41

4 years ago
Created attachment 723390 [details]
Xul Tabs Test
Attachment #723389 - Attachment is obsolete: true

Comment 42

4 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 43

4 years ago
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
Attachment #718608 - Attachment is obsolete: true
Attachment #718609 - Attachment is obsolete: true
Attachment #727268 - Flags: review?(bzbarsky)
Comment on attachment 727268 [details] [diff] [review]
Disable XBL scopes for XUL-whitelisted domains. v1

r=me
Attachment #727268 - Flags: review?(bzbarsky) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b75fab8561b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/775abfe4876f
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
Depends on: 853747
https://hg.mozilla.org/mozilla-central/rev/c5ad6568024d
https://hg.mozilla.org/mozilla-central/rev/fddb99719062
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 854019
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 52

4 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)
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?
Whiteboard: [leave uplift for bholley]
https://tbpl.mozilla.org/?tree=Try&rev=a34ef87bd895
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+
(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
status-firefox21: affected → fixed
You need to log in before you can comment on or make changes to this bug.