754164 - Bug 650353 breaks Lightning with [TypeError: can't wrap XML objects] due to e4x usage

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Matthew Turnbull [Bluefang]]

Filed following discussion in Bug 752281.

Lightning is broken by CPG because it uses E4X.

Comment 1

[Stefan Sitter [:ssitter]]

This is the original error report from Bug 752281:

--------------------------------------
Timestamp: 05/06/2012 12:23:59 AM
Error: Assert failed: [Exception... "'TypeError: can't wrap XML objects' when calling method: [calICalDavCalendar::refresh]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame :: file:///Data/Profiles/Thunderbird/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calCompositeCalendar.js :: cCC_refresh :: line 378"  data: no]
2: [null:0] null
3: [file:///Data/Profiles/Thunderbird/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calCompositeCalendar.js:381] cCC_refresh
4: [null:0] null
5: [chrome://calendar/content/calendar-common-sets.js:402] cC_doCommand
6: [chrome://global/content/globalOverlay.js:98] goDoCommand
7: [chrome://messenger/content/messenger.xul:1] oncommand
8: [null:0] null

Source File: resource://calendar/modules/calUtils.jsm -> file:///Data/Profiles/Thunderbird/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calUtils.js
Line: 1105
--------------------------------------

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Oh don't tell me you removed e4x support, or at least effectively broke it with CPG? I was told there shall be an easy replacement to e4x before it is to be removed? And by easy I don't mean document.createElement().

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Workaround - v1

Usually I shouldn't do things like this while on vacation, but this bug personally aggravates me ;-)

This patch works around by serializing the e4x directly in the function it is created in, not passing e4x to other compartments.

I still need an answer about a replacement for e4x. Bluefang, maybe you could CC some people and/or tell me what the promised replacement is?

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Workaround - v1

Forgot to qrefresh, now with gdata changes too.

Comment 5

[Matthew Mecca [:mmecca]]

I'm still getting a DAV_NOT_DAV error here 
http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js#1619

It looks like cal.safeNewXML is still affected.

Comment 6

[Bruno Browning]

I don't think that cal.safeNewXML is the only problem. Looks to me like e4x is broken in js-land, so all our multistatus..D::whatever parsing is broken as well. Might be time for xpath or similar.

Comment 7

[Matthew Mecca [:mmecca]]

Attachment: Workaround for safeNewXML

I was able to get a Google CAlDAV calendar working again with Workaround v1 + this fix, but a similar fix would be needed for the GData and probably ICS providers - is there a safe place we could move the safeNewXML function to that could be shared between providers without crossing compartments?

Comment 8

[Matthew Turnbull [Bluefang]]

(In reply to Philipp Kewisch [:Fallen] (away until May 28th) from comment #3)
> I still need an answer about a replacement for e4x. Bluefang, maybe you
> could CC some people and/or tell me what the promised replacement is?

Sorry, I'm just an end-user who files bugs. All I know about the issue is what's said in Bug 752281. E4X is broken because it's not allowed for cross-compartment wrappers.

It looks like E4X has been on the road to depreciation and removal for at least 1/2 a year. Given that only Mozilla and Adobe support it, and the complexity and security considerations, that doesn't sound like a bad thing to me. Here's some relevant stuff I've found:

Bug 485791 - Self-host the E4X runtime in JS using proxies
Bug 613142 - "can't wrap XML objects"
Bug 695577 - E4X syntax should not be accepted in ES5 strict mode
https://groups.google.com/d/topic/mozilla.dev.tech.js-engine/4iMtEASXxL8/discussion
https://developer.mozilla.org/en/E4X

It looks like JXON is the recommended alternative:

https://developer.mozilla.org/en/JXON

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I see, but using JXON feels a bit overkill just for parsing responses:

string -> dom tree -> jxon / js object

If JXON were native and there were a native string -> js object parser, that might look differently.

Also, since we need to use namespaces, the parser on MDN doesn't work for us, and even if it supported that, there would be no selector *::node.

Looks like I need to complain on the newsgroups again.

mmecca, I agree your workaround is needed, I stumbled upon this after resetting the cache too. I guess we could use this as a quick workaround, but on the other hand we need to replace e4x anyway, and from what Bluefang referenced and personal research, it seems JXON is all we get as an alternative.

I think this is the best way to do this is:

Parsing String -> Javascript:
* Maybe we can use the SAX parser to directly create the js object tree from 
  parsing the string.
* Depending on the situation, maybe we can skip creating the whole object tree and 
  just pull out the relevant information in the sax parser.
    - Downside is that this pulls apart the code a lot, no more simple for each
      iterations.


Parsing XML queries -> String
* Some of the queries are static, we can just put '' around the XML and create it
  as a string
* Others require some manipulation. We can move the String -> DOM transformation
  somewhere into the global scope and then just use simple DOM accessor methods 
  (firstChild, childNodes, ...) to get to the nodes we need to change.


mmecca, what do you think?

Comment 10

[Matthew Mecca [:mmecca]]

That sounds reasonable.

IIUC e4x isn't being dropped completely right now, and should continue to work as long as we can keep it contained to individual compartments. I think the safest bet is to use the workarounds for now to get everything working again, and buy some time to put the replacement in place and make sure it's tested thoroughly.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

BTW: I tried some mad hacks using Components.utils.getGlobalForObject() and friends, but cross compartment wrappers and the fact that func.caller is broken across jsm boundaries (bug 586453) seem to be bashing this.

Before I spend more time trying to find a hack, I'll rather spend time getting rid of e4x. If you want a quick fix feel free to fold the patches and push them, r=me.

Comment 12

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Get rid of e4x - WiP - v1

This patch takes a first stab at getting rid of e4x. I've tested this with a Zimbra and SOGo server and it works (even freebusy), but I couldn't test all queries (i.e principal-property-search). Maybe you have a different caldav server handy for testing? Please stop me from writing a caldav fakeserver just to fix this bug ;-)

If this works out fine, I still need to find a solution for the gdata provider, as it uses e4x very much more extensively. I guess it will be similar, or I would rewrite to use gdata's JSON output (yuck, that almost means rewrite to use the latest gdata api). Maybe I'll just move safeNewXML to the gdata compartment and take care later.

Matthew, what do you think of this patch?

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Alas, I need to look though calDavRequestHandlers too. It mostly uses SAX, but it does use e4x to put together some of the queries.

Comment 14

[Matthew Mecca [:mmecca]]

(In reply to Philipp Kewisch [:Fallen] (away until May 28th) from comment #12)

This looks good.

> This patch takes a first stab at getting rid of e4x. I've tested this with a
> Zimbra and SOGo server and it works (even freebusy), but I couldn't test all
> queries (i.e principal-property-search). Maybe you have a different caldav
> server handy for testing? Please stop me from writing a caldav fakeserver
> just to fix this bug ;-)

Seems to be working well with Google Calendar via CalDAV, unfortunately I don't have another CalDAV server to test with at the moment.

> If this works out fine, I still need to find a solution for the gdata
> provider, as it uses e4x very much more extensively. I guess it will be
> similar, or I would rewrite to use gdata's JSON output (yuck, that almost
> means rewrite to use the latest gdata api). Maybe I'll just move safeNewXML
> to the gdata compartment and take care later.

I think the safeNewXML workaround should work for now. We're going to have to move to the new version of the gdata api at some point anyway, maybe we can get task support as a bonus if we look into that soon.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

I think this is ready for review. I'd appreciate if you could do some thorough testing to make sure this doesn't break anything :-)

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Argh, it seems something is not working for me, getting empty calendars with Zimbra.

Comment 17

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v3

Oh, that was easy. I accidentally put the <D:href> elements into the <D:prop>.

Comment 18

[Matthew Mecca [:mmecca]]

Comment on attachment 625453 [details] [diff] [review]
Fix - v3

Review of attachment 625453 [details] [diff] [review]:
-----------------------------------------------------------------

CalDAV via Google Calendar and WebDAV seem to be working fine. I'm still getting the "can't wrap XML objects" error with the GData Provider, but if you want to take care of that in a followup bug I'm fine with that. r=mmecca

::: calendar/base/modules/Makefile.in
@@ +55,4 @@
>      calPrintUtils.jsm \
>      calProviderUtils.jsm \
>      calUtils.jsm \
> +	calXMLUtils.jsm \

Looks like a tab snuck in here.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Matthew Mecca [:mmecca] from comment #18)
> Comment on attachment 625453 [details] [diff] [review]
> Fix - v3
> 
> Review of attachment 625453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> CalDAV via Google Calendar and WebDAV seem to be working fine. I'm still
> getting the "can't wrap XML objects" error with the GData Provider, but if
> you want to take care of that in a followup bug I'm fine with that. r=mmecca

Oh boy, this sucks: each script loaded via subscript loader has its own global (which I guess is expected), but that means I can't use an e4x object created in calGoogleUtils.js anywhere in calGoogleCalendar.js.

This means I will have to change everything afterall :-( Off to a separate bug, reverting the gdata changes in the above patch and leaving it broken.

> 
> ::: calendar/base/modules/Makefile.in
> @@ +55,4 @@
> >      calPrintUtils.jsm \
> >      calProviderUtils.jsm \
> >      calUtils.jsm \
> > +	calXMLUtils.jsm \
> 
> Looks like a tab snuck in here.

Good catch, fixed.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset a182ec1d232f