The default bug view has changed. See this FAQ.

Various issues in webSearchProvider.js

RESOLVED FIXED in Thunderbird 14.0

Status

Thunderbird
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Chris Coulson, Assigned: standard8)

Tracking

Trunk
Thunderbird 14.0

Thunderbird Tracking Flags

(thunderbird11-, thunderbird12+ fixed, thunderbird13 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 603755 [details] [diff] [review]
Various fixes for webSearchProvider.js

I'm trying to get the web search feature working in Ubuntu, and hit some issues with webSearchProvider.js.

- "Cu" isn't defined anywhere, which prevents the component from loading at all
- The definition of NS_XPCOM_CURRENT_PROCESS_DIR  has a typo
- The logic in appendFileKey() for checking if the location exists is reversed
- The category stanza is incorrect in mailComponents.manifest, as nsDirectoryService::RegisterCategoryProviders() doesn't understand "service,@mozilla.org/mail/web-search-provider;1". This is specific to nsAppStartupNotifier, which handles the "app-startup" category.
- getFiles() returns non-existant searchplugin directories for extensions (even packed extensions), which makes nsSearchService blow up

This doesn't affect mozilla.org builds, as the searchplugin location in the application directory is found by the default provider. In Ubuntu, we want to install the searchplugins in to distribution/searchplugins, which gives us the ability to have language packs provide localized plugins
(Reporter)

Updated

5 years ago
Attachment #603755 - Flags: review?(mbanner)
(Reporter)

Comment 1

5 years ago
I should clarify - by "doesn't affect mozilla.org builds", I mean that the opensearch stuff still works by virtue of webSearchProvider.js not loading at all, but the bugs aren't specific to our builds :)
(Assignee)

Updated

5 years ago
tracking-thunderbird11: --- → ?
tracking-thunderbird12: --- → ?
(Assignee)

Updated

5 years ago
tracking-thunderbird11: ? → -
tracking-thunderbird12: ? → +
(Assignee)

Comment 2

5 years ago
Comment on attachment 603755 [details] [diff] [review]
Various fixes for webSearchProvider.js

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

Ok, this looks great and I've verified all the variations, thanks for the patch.
Attachment #603755 - Flags: review?(mbanner) → review+
(Assignee)

Updated

5 years ago
Assignee: nobody → chrisccoulson
(Assignee)

Comment 3

5 years ago
Checked in:

http://hg.mozilla.org/comm-central/rev/29d89350dce1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
(Assignee)

Comment 4

5 years ago
Comment on attachment 603755 [details] [diff] [review]
Various fixes for webSearchProvider.js

[Triage Comment]
We should take this on aurora & beta as well, maybe esr - it fixes something that is broken and it'll provide greater flexibility for extensions, distributions etc.
Attachment #603755 - Flags: approval-comm-beta+
Attachment #603755 - Flags: approval-comm-aurora+
(Assignee)

Comment 5

5 years ago
Checked in:

http://hg.mozilla.org/releases/comm-aurora/rev/7368ce5fc056
http://hg.mozilla.org/releases/comm-beta/rev/491ada0b739c
status-thunderbird12: --- → fixed
status-thunderbird13: --- → fixed
(Assignee)

Comment 6

5 years ago
I had to push a unit test bustage fix for this.

Now the file is actually working, xpconnect didn't like the throw Components.results.NS_ERROR_NOT_IMPLEMENTED; in getFile() - it would log an error to the console and that was upsetting some of the tests that relied on no errors being logged.

Therefore I changed this just to return null which still is handled correctly but avoids the warning:

http://hg.mozilla.org/comm-central/rev/c20698efdd88
http://hg.mozilla.org/releases/comm-aurora/rev/eb3c266e224e
http://hg.mozilla.org/releases/comm-beta/rev/37248c37f757
(Assignee)

Comment 7

5 years ago
Unfortunately I had to back this out due to continuing unit test failures:

http://hg.mozilla.org/comm-central/rev/15769b1a16d0
http://hg.mozilla.org/releases/comm-aurora/rev/c87d0eee72a8
http://hg.mozilla.org/releases/comm-beta/rev/83fc75101b40
Status: RESOLVED → REOPENED
status-thunderbird12: fixed → ---
status-thunderbird13: fixed → ---
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Attachment #603755 - Flags: approval-comm-beta-
Attachment #603755 - Flags: approval-comm-beta+
Attachment #603755 - Flags: approval-comm-aurora-
Attachment #603755 - Flags: approval-comm-aurora+
(Reporter)

Comment 8

5 years ago
Oh, my. Brace yourself for this :)

So, the test failures aren't caused by any errors in webSearchProvider.js. It actually turns out that the exceptions that are seen in the test failures occurred previously. Eg, here, nsMsgDBFolder::GetStringProperty() returns NS_MSG_ERROR_FOLDER_MISSING:


TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux64-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/db/gloda/test/unit/test_index_messages_imap_offline.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | ../../../../resources/logHelper.js | Error console says [stackFrame [Exception... "Component returned failure code: 0x80550007 [nsIMsgFolder.getStringProperty]"  nsresult: "0x80550007 (<unknown>)"  location: "JS frame :: resource:///modules/gloda/datastore.js :: gloda_ds_mapFolderURI :: line 2084"  data: no]] - See following stack:

This is propagated as an exception in JS all the way back to xpconnect, and happened even without my change. What normally happens is that nsXPCWrappedJSClass::CheckForException() delivers this error to the error reporter registered by xpcshell (my_ErrorReporter()). The error is logged by xpcshell and then basically swallowed and goes no further.

With my change committed, the error is no longer being handled by the xpcshell error handler, because the error handler hasn't been set correctly on the JSContext. This means that xpconnect delivers it to the console service instead, where it is caught by the test harness --> fail.

The reason this is happening is because when xpcshell starts xpcom, nsDirectoryService loads the providers registered using the xpcom-directory-providers category. Now we are registering a provider implemented in JS, this is causing the JSContext to be created (in mozJSComponentLoader::ReallyInit()) *before* xpcshell has had a chance to set up its context callback (using JS_SetContextCallback), which it uses to override the error reporter on newly created JSContext's.
Status: REOPENED → NEW
status-thunderbird12: --- → fixed
status-thunderbird13: --- → fixed
(Reporter)

Comment 9

5 years ago
So, I'm not sure how to proceed here, other than "rewrite webSearchProvider.js" in C++ ;)
(Reporter)

Comment 10

5 years ago
(In reply to Chris Coulson from comment #8)
> The error is logged by
> xpcshell and then basically swallowed and goes no further.
> 

What I meant to say here is that it is logged by xpcshell and not to the console service. Of course, the error still propagates through xpconnect to native code
(Reporter)

Comment 11

5 years ago
I'm not sure how I managed to change all of the status flags, but that wasn't intentional :/

Updated

5 years ago
Status: NEW → ASSIGNED
status-thunderbird12: fixed → ---
status-thunderbird13: fixed → ---
(Assignee)

Comment 12

5 years ago
Created attachment 612484 [details] [diff] [review]
Switch to C++

Given the brokenness in xpcshell, but the fact I'd also like to get this fixed for TB 12, I'm proposing that we switch to c++ for now. This is a complete work around for the xpcshell failures.

The code here is therefore directly copied from Firefox's version, with the non-searchplugin stuff removed.

As it is a direct copy, I've kept the license headers the same rather than switching to mpl2 - we can do that when the others are done.

This passes the tests locally for me, but I've pushed to try server anyway:

http://hg.mozilla.org/try-comm-central/rev/fab4fb82431c

At some point in the future, we can perhaps investigate more and see if there's a way we can switch this to javascript again.
Assignee: chrisccoulson → mbanner
Attachment #603755 - Attachment is obsolete: true
Attachment #612484 - Flags: review?(dbienvenu)
Attachment #612484 - Flags: feedback?(chrisccoulson)
(Reporter)

Comment 13

5 years ago
Comment on attachment 612484 [details] [diff] [review]
Switch to C++

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

Looks good to me!
Attachment #612484 - Flags: feedback?(chrisccoulson) → feedback+

Comment 14

5 years ago
Comment on attachment 612484 [details] [diff] [review]
Switch to C++

gloda tests are a bit flaky, but I think this is OK.
Attachment #612484 - Flags: review?(dbienvenu) → review+
(Assignee)

Comment 15

5 years ago
Checked in:

http://hg.mozilla.org/comm-central/rev/e6d6ed8fee9d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

5 years ago
Comment on attachment 612484 [details] [diff] [review]
Switch to C++

[Triage Comment]
Attachment #612484 - Flags: approval-comm-beta+
Attachment #612484 - Flags: approval-comm-aurora+
(Assignee)

Comment 17

5 years ago
Checked in:

http://hg.mozilla.org/releases/comm-aurora/rev/400d35483957
http://hg.mozilla.org/releases/comm-beta/rev/8db3dc572a19
status-thunderbird12: --- → fixed
status-thunderbird13: --- → fixed
You need to log in before you can comment on or make changes to this bug.