Closed Bug 481450 Opened 15 years ago Closed 15 years ago

new XML() to cal.safeNewXML()

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pgoerzen, Assigned: pgoerzen)

Details

Attachments

(1 file, 1 obsolete file)

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
Attached 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?
(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
(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.
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
Closed: 15 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.