Various issues in webSearchProvider.js

RESOLVED FIXED in Thunderbird 14.0


8 years ago
Last year


(Reporter: chrisccoulson, Assigned: standard8)


Thunderbird 14.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird11-, thunderbird12+ fixed, thunderbird13 fixed)



(1 attachment, 1 obsolete attachment)

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,;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 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
Attachment #603755 - Flags: review?(mbanner)
I should clarify - by "doesn't affect 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 :)
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: nobody → chrisccoulson
Checked in:
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
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+
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:
Unfortunately I had to back this out due to continuing unit test failures:
Resolution: FIXED → ---
Attachment #603755 - Flags: approval-comm-beta-
Attachment #603755 - Flags: approval-comm-beta+
Attachment #603755 - Flags: approval-comm-aurora-
Attachment #603755 - Flags: approval-comm-aurora+
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.
So, I'm not sure how to proceed here, other than "rewrite webSearchProvider.js" in C++ ;)
(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
I'm not sure how I managed to change all of the status flags, but that wasn't intentional :/
Posted patch Switch to C++Splinter Review
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:

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)
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 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+
Checked in:
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Comment on attachment 612484 [details] [diff] [review]
Switch to C++

[Triage Comment]
Attachment #612484 - Flags: approval-comm-beta+
Attachment #612484 - Flags: approval-comm-aurora+
Depends on: 736762
You need to log in before you can comment on or make changes to this bug.