Last Comment Bug 733802 - Various issues in webSearchProvider.js
: Various issues in webSearchProvider.js
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 14.0
Assigned To: Mark Banner (:standard8)
Depends on:
  Show dependency treegraph
Reported: 2012-03-07 09:21 PST by Chris Coulson
Modified: 2012-04-16 02:43 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Various fixes for webSearchProvider.js (3.21 KB, patch)
2012-03-07 09:21 PST, Chris Coulson
standard8: review+
standard8: approval‑comm‑aurora-
standard8: approval‑comm‑beta-
Details | Diff | Splinter Review
Switch to C++ (22.62 KB, patch)
2012-04-05 03:10 PDT, Mark Banner (:standard8)
mozilla: review+
chrisccoulson: feedback+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description User image Chris Coulson 2012-03-07 09:21:15 PST
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,;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
Comment 1 User image Chris Coulson 2012-03-07 12:24:04 PST
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 2 User image Mark Banner (:standard8) 2012-03-20 06:31:20 PDT
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.
Comment 3 User image Mark Banner (:standard8) 2012-03-20 06:59:40 PDT
Checked in:
Comment 4 User image Mark Banner (:standard8) 2012-03-20 07:00:47 PDT
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.
Comment 6 User image Mark Banner (:standard8) 2012-03-20 08:22:31 PDT
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:
Comment 7 User image Mark Banner (:standard8) 2012-03-20 13:19:03 PDT
Unfortunately I had to back this out due to continuing unit test failures:
Comment 8 User image Chris Coulson 2012-03-21 13:51:23 PDT
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.
Comment 9 User image Chris Coulson 2012-03-21 13:54:05 PDT
So, I'm not sure how to proceed here, other than "rewrite webSearchProvider.js" in C++ ;)
Comment 10 User image Chris Coulson 2012-03-21 13:59:40 PDT
(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
Comment 11 User image Chris Coulson 2012-03-21 14:13:16 PDT
I'm not sure how I managed to change all of the status flags, but that wasn't intentional :/
Comment 12 User image Mark Banner (:standard8) 2012-04-05 03:10:10 PDT
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:

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.
Comment 13 User image Chris Coulson 2012-04-05 08:48:28 PDT
Comment on attachment 612484 [details] [diff] [review]
Switch to C++

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

Looks good to me!
Comment 14 User image David :Bienvenu 2012-04-05 13:02:11 PDT
Comment on attachment 612484 [details] [diff] [review]
Switch to C++

gloda tests are a bit flaky, but I think this is OK.
Comment 15 User image Mark Banner (:standard8) 2012-04-13 07:53:29 PDT
Checked in:
Comment 16 User image Mark Banner (:standard8) 2012-04-16 01:12:19 PDT
Comment on attachment 612484 [details] [diff] [review]
Switch to C++

[Triage Comment]

Note You need to log in before you can comment on or make changes to this bug.