new XML() to cal.safeNewXML()

RESOLVED FIXED in 1.0b1

Status

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: pgoerzen, Assigned: pgoerzen)

Tracking

unspecified
1.0b1

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6) Gecko/2009020409 Iceweasel/3.0.6 (Debian-3.0.6-1)
Build Identifier: 

There are several occurrences of new XML() that ought to use the wrapper.

Reproducible: Always
Assignee

Comment 1

10 years ago
Posted patch Use the safeNewXML wrapper (obsolete) — Splinter Review
Tested the ICS code, and it works for me. I don't have a Google account so, I couldn't test those providers. Also, I'm guessing that for the Google occurrences, aData.substring(38) can be reduced to aData since safeNewXML takes care of stripping out the <?xml...
Assignee: nobody → pgoerzen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #365465 - Flags: review?(philipp)
Comment on attachment 365465 [details] [diff] [review]
Use the safeNewXML wrapper

>             // A feed was passed back, parse it. Due to bug 336551 we need to
>             // filter out the <?xml...?> part.
>-            var xml = new XML(aData.substring(38));
>+            var xml = cal.safeNewXML(aData.substring(38));
...
>-                if (str.indexOf("<?xml ") == 0) {
>-                    str = str.substring(str.indexOf('<', 2));
>-                }
>-                var multistatus = new XML(str);
>+                var multistatus = cal.safeNewXML(str);

We can even go a step further and make safeNewXML remove all types of <?xml ...?> processing instructions. I'd use regex to do so. Mind posting a new patch to do so?
Assignee

Comment 3

10 years ago
(In reply to comment #2)
> We can even go a step further and make safeNewXML remove all types of <?xml
> ...?> processing instructions. I'd use regex to do so. Mind posting a new patch
> to do so?

I'm a little bit confused. It looks like safeNewXML already removes <?xml. . .?>:
return new XML(aStr.replace(/^\s*<\?xml[^>]*>/g, "").trimRight());

Do you want to remove all patterns of the form <?...?> ? If so, I'll modify the regex to do that.

Thanks,
Peter
Assignee

Comment 4

10 years ago
(In reply to comment #3)
> Do you want to remove all patterns of the form <?...?> ? If so, I'll modify the
> regex to do that.

Actually, it looks like XML.ignoreProcessingInstructions is turned on by default, which should ignore all processing instructions except the <?xml. . .?> element.
Oh sorry, I didn't read safeNewXML in detail. What I noticed though that in the calling code, the xml part is removed by hand. In gdata for example, I used aData.substring(38) to remove the <?xml part, this could be shortened to just pass aData since safeNewXML takes care. The comment about the respective bug should be moved into safeNewXML.

The same goes for the caldav code that removes it with substring/indexOf. str could also be passed directly here.
Assignee

Comment 6

10 years ago
Ok, I've changed aData.substring(38) to aData and documented the bug number in safeNewXML. All the substring/indexOf code appears to be removed from caldav already.
Attachment #365465 - Attachment is obsolete: true
Attachment #365465 - Flags: review?(philipp)
Attachment #365654 - Flags: review?(philipp)
Thanks for taking care!

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6fadebfb15d1>

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Comment on attachment 365654 [details] [diff] [review]
remove substring(38), document bug number in safeNewXML

This patch got reviewed and checked in, but not marked as r+.
Attachment #365654 - Flags: review?(philipp) → review+
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.