1679464 - nsAbQueryStringToExpression::ParseExpression fails when given a ? at the start of the query

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Mark Banner (:standard8)]

In browser.contacts.quickSearch we are formulating a query string:

https://searchfox.org/comm-central/rev/f6b563aaf622ff6fe9836cd8e6ffe9b2851a63b6/mail/components/extensions/parent/ext-addressBook.js#566-569

Unfortunately generateQueryURI adds a question mark to the beginning of the string. For the JavaScript based address books, this is fine, but it breaks the address books that use compiled code as nsAbQueryStringToExpression::ParseExpression isn't accepting it.

I think the simplest way around this if for ParseExpression to accept the ? like the JavaScript implementation does:

https://searchfox.org/comm-central/rev/f6b563aaf622ff6fe9836cd8e6ffe9b2851a63b6/mailnews/addrbook/modules/AddrBookDirectory.jsm#500-502

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1679464 - Extend nsAbQueryStringToExpression::ParseExpression to handle '?' at the start of queries. r?darktrojan!

Comment 2

[Ben Bucksch (:BenB)]

The bug is actually even earlier than that: The first character of an LDAP query string must always be a (, so a leading ? is always erroneous. Whatever generates the ? should be fixed, too.

But you're right with your patch - the code should be tolerant. (Internet protocol principle: Conservative in sending, tolerant in receiving.)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5c8ea2dad760
Extend nsAbQueryStringToExpression::ParseExpression to handle '?' at the start of queries. r=darktrojan

Comment 4

[[e-mail]]

Attachment: Alternative approach

I double-checked and all of the consumers of generateQueryURI subsequently remove the leading ? from the resulting string at some point, making it pointless to prepend it in the first place, so we could just remove it. (I also removed URI from the name, as we're not using the string as part of a URI any more.)

Comment 5

[Ben Bucksch (:BenB)]

Thanks, Mark, for the patch and for landing it.

REOPENing (it was closed automatically by the bot), because the root cause isn't fixed yet.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 9190265 [details] [diff] [review]
Alternative approach

I'm fine with taking an alternate approach assuming everything still works. The patch will probably want to revert the C++ changes as well.

Though I did think we generally handled follow-ups in separate bugs...

Comment 7

[Mark Banner (:standard8)]

Re-marking as fixed, as this did fix an issue, even if it is not the best for the root cause. I also want to uplift just the simple patch, so for tracking purposes, any follow-ups should be on separate bugs to save confusion.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 9189887 [details]
Bug 1679464 - Extend nsAbQueryStringToExpression::ParseExpression to handle '?' at the start of queries. r?darktrojan!

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: If the user has current versions of Exquilla installed (78.0.5) the WebExtension Contacts API will be broken for quick searches - and hence it can break other add-ons.
Testing completed (on c-c, etc.): Landed on c-c.
Risk to taking this patch (and alternatives if risky): Low, simple fix that makes nsAbQueryStringToExpression::ParseExpression handle an extra edge case.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9189887 [details]
Bug 1679464 - Extend nsAbQueryStringToExpression::ParseExpression to handle '?' at the start of queries. r?darktrojan!

I think you meant 78.

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9189887 [details]
Bug 1679464 - Extend nsAbQueryStringToExpression::ParseExpression to handle '?' at the start of queries. r?darktrojan!

[Triage Comment]
Approved for beta

Comment 11

[Wayne Mery (:wsmwk)]

Comment on attachment 9189887 [details]
Bug 1679464 - Extend nsAbQueryStringToExpression::ParseExpression to handle '?' at the start of queries. r?darktrojan!

[Triage Comment]
Approved for esr78

Comment 12

[Rob Lemley [:rjl]]

Thunderbird 78.6.1:
https://hg.mozilla.org/releases/comm-esr78/rev/8207c264e583