Closed
Bug 733802
Opened 12 years ago
Closed 12 years ago
Various issues in webSearchProvider.js
Categories
(Thunderbird :: General, defect)
Thunderbird
General
Tracking
(thunderbird11-, thunderbird12+ fixed, thunderbird13 fixed)
RESOLVED
FIXED
Thunderbird 14.0
People
(Reporter: chrisccoulson, Assigned: standard8)
References
Details
Attachments
(1 file, 1 obsolete file)
22.62 KB,
patch
|
Bienvenu
:
review+
chrisccoulson
:
feedback+
standard8
:
approval-comm-aurora+
standard8
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Attachment #603755 -
Flags: review?(mbanner)
Reporter | ||
Comment 1•12 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•12 years ago
|
tracking-thunderbird11:
--- → ?
tracking-thunderbird12:
--- → ?
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 2•12 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•12 years ago
|
Assignee: nobody → chrisccoulson
Assignee | ||
Comment 3•12 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/29d89350dce1
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Assignee | ||
Comment 4•12 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•12 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•12 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•12 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•12 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•12 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.
Reporter | ||
Comment 9•12 years ago
|
||
So, I'm not sure how to proceed here, other than "rewrite webSearchProvider.js" in C++ ;)
Reporter | ||
Comment 10•12 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•12 years ago
|
||
I'm not sure how I managed to change all of the status flags, but that wasn't intentional :/
Updated•12 years ago
|
Assignee | ||
Comment 12•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/e6d6ed8fee9d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 16•12 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•12 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.
Description
•