1097364 - Support OpenSearchDescription in search engine parser

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Search Activity, defect)

Description

[:Margaret Leibovic]

The search activity will crash if we get an engine that uses <OpenSearchDescription> as the root tag instead of <SearchPlugin>.

First of all, I should make sure we won't crash if we fail to parse a plugin. Second of all, I can just add support for this tag as well.

This doesn't affect search engines we ship, but it can affect third-party engines installed by the user.

Comment 1

[:Margaret Leibovic]

Attachment: Support <OpenSearchDescription> as the root tag of a search plugin

The Gecko search service treats <SearchPlugin> and <OpenSearchDescription> exactly the same, so we can do the same thing.

The search service also verifies the namespaces, but I think we're okay with that.

I can't use require to verify that a tag is one of two types, so I just modeled this after the way the docs describe the implementation:
https://developer.android.com/reference/org/xmlpull/v1/XmlPullParser.html#require%28int,%20java.lang.String,%20java.lang.String%29

Comment 2

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #1)

> The search service also verifies the namespaces, but I think we're okay with
> that.

I intended to say "without that".

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 8521058 [details] [diff] [review]
Support <OpenSearchDescription> as the root tag of a search plugin

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

(In reply to :Margaret Leibovic from comment #0)
> The search activity will crash if we get an engine that uses
> <OpenSearchDescription> as the root tag instead of <SearchPlugin>.

Weird, I would have expected it to throw an XmlPullParserException, which we catch. What was happening in the crash?

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngine.java
@@ +78,5 @@
> +            throw new XmlPullParserException("Expected start tag: " + parser.getPositionDescription());
> +        }
> +
> +        final String name = parser.getName();
> +        if (!"SearchPlugin".equals(name)  && !"OpenSearchDescription".equals(name)) {

Nit: remove extra space before &&

@@ +82,5 @@
> +        if (!"SearchPlugin".equals(name)  && !"OpenSearchDescription".equals(name)) {
> +            throw new XmlPullParserException("Expected <SearchPlugin> or <OpenSearchDescription> as root tag: "
> +                + parser.getPositionDescription());
> +        }
> +     

Nit: ws

Comment 4

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Comment on attachment 8521058 [details] [diff] [review]
> Support <OpenSearchDescription> as the root tag of a search plugin
> 
> Review of attachment 8521058 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to :Margaret Leibovic from comment #0)
> > The search activity will crash if we get an engine that uses
> > <OpenSearchDescription> as the root tag instead of <SearchPlugin>.
> 
> Weird, I would have expected it to throw an XmlPullParserException, which we
> catch. What was happening in the crash?

Oh, this was happening while testing a patch for bug 1091728. I think I saw this exception in the log, but then an NPE later was the real cause of the crash. I'm still iterating on that patch, so I should make sure to account for createEngine failing.

Comment 5

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/7db83e0a4137

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7db83e0a4137

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Is this needed for Fx35?

Comment 8

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Is this needed for Fx35?

Yes, it's low risk, let's uplift it (will prevent problems with third party engines).

Comment 9

[:Margaret Leibovic]

Comment on attachment 8521058 [details] [diff] [review]
Support <OpenSearchDescription> as the root tag of a search plugin

Approval Request Comment
[Feature/regressing bug #]: search activity
[User impact if declined]: some third party search engines won't work
[Describe test coverage new/current, TBPL]: no automated tests, tested locally and landed on m-c
[Risks and why]: low-risk, small modification to search engine xml parser
[String/UUID change made/needed]: none

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8368c5930744