Closed Bug 1003968 Opened 11 years ago Closed 5 years ago

avoid Exists() calls for search plugin directories in the browser directory provider

Categories

(Firefox :: Search, defect, P2)

Product:
Firefox
Component:
Search
Version:
29 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: Gavin, Assigned: ruthra, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: main-thread-io, perf, Whiteboard: [good first bug][lang=c++][fxsearch] [fxperf:p2])

Attachments

(5 files, 3 obsolete files)

1003968.diff
(deleted), patch
rvitillo
: review-
Details | Diff | Splinter Review
1003968v2.diff
(deleted), patch
rvitillo
: review-
Details | Diff | Splinter Review
1003968_u1.diff
(deleted), patch
ruthra
: review+
ruthra
: feedback+
Details | Diff | Splinter Review
Bug 1003968 - avoid Exists() calls for search plugin directories in the browser directory provider
(deleted), text/x-phabricator-request
Details
Bug 1003968 - avoid Exists() calls for search plugin directories in the browser directory provider
(deleted), text/x-phabricator-request
Details
AppendDistroSearchDirs and DirectoryProvider::AppendingEnumerator::GetNext in browser/components/dirprovider/DirectoryProvider.cpp call exists() on directories before returning them - that might not be necessary, and would allow us to avoid some main-thread-IO.
Flags: firefox-backlog+
Whiteboard: [good first bug][lang=c++]
Hello, can I work on this problem. This seems to be sutable for a newcomer like me.
Clement, feel free to go ahead and start working on it :) You can ask questions in the bug or on irc in #perf.
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][mentor=rvitillo]
To elaborate on my "that might not be necessary" comment: The basic setup is that the search service calls the directory service to get a list directories from which it should attempt to load search plugins (NS_APP_SEARCH_DIR_LIST). The code that builds that list is in DirectoryProvider::GetFiles: http://hg.mozilla.org/mozilla-central/annotate/ab16ec3fd6b2/browser/components/dirprovider/DirectoryProvider.cpp#l212 There, AppendFileKey, AppendDistroSearchDirs, and AppendingEnumerator::GetNext are all called to add various directories to the list of directories returned, and they check whether these directories exist before adding them (by calling nsIFile->Exists). This ensures that all of the directories returned to the search service for NS_APP_SEARCH_DIR_LIST exist. What we'd like to do is eliminate the Exists() calls in AppendFileKey, AppendDistroSearchDirs, and AppendingEnumerator::GetNext, and instead just return all possible locations to the search service regardless of whether they exist. Then, ensure that the search service handles that properly by failing gracefully when trying to load plugins from a non-existent directory. The search service has two initialization paths, one synchronous and one asynchronous. Both paths end up requesting the list of directories, and then iterating over them to load engines from them. They both eventually end up calling "loadEnginesFromDir" functions for each returned directory: Sync version: http://hg.mozilla.org/mozilla-central/annotate/ab16ec3fd6b2/toolkit/components/search/nsSearchService.js#l3296 Async version: http://hg.mozilla.org/mozilla-central/annotate/ab16ec3fd6b2/toolkit/components/search/nsSearchService.js#l3342 After removing all of the relevant Exists() calls in DirectoryProvider.cpp, we need to ensure that those functions fail gracefully if aDir is non-existent. It looks like they currently do not. Making them handle that gracefully probably involves adding some error handling to catch the directory iteration failing - might be as simple as adding a try/catch in both cases.
Clement, are you still working on this bug? I didn't hear from you for a while.
Flags: needinfo?(c.charasson)
Mentor: rvitillo
Whiteboard: [good first bug][lang=c++][mentor=rvitillo] → [good first bug][lang=c++]
Hi, I am a beginner and would like to work on this bug. Can anyone assign it to me?
Athira, feel free to go ahead and start working on it :)
Flags: needinfo?(c.charasson)
Thank you.After removing the Exists() call what change has to be the in the code.I didn't understand about error handling.
Athira, I saw you wrote me on IRC, let's continue the discussion over there.
Hi. I'd like to work on this bug if noone is working on it. Can this be assigned to me ?
It's all yours! I just came back from PTO so feel free to contact me on IRC.
Attached patch 1003968.diff (deleted) — — Splinter Review
This patch removes the Exists() calls from DirectoryProvider.cpp. Project builds successfully. I also ran all the xpcshell tests and all of them passed. Could you please tell me how exactly the functions in nsSearchService.js are failing and what triggers them?
Comment on attachment 8477891 [details] [diff] [review] 1003968.diff Review of attachment 8477891 [details] [diff] [review]: ----------------------------------------------------------------- As noted by Gavin in comment 3, the search service has two initialization paths, one synchronous and one asynchronous. For instance, in the synchronous path, an exception is thrown at http://hg.mozilla.org/mozilla-central/annotate/ab16ec3fd6b2/toolkit/components/search/nsSearchService.js#l3302 when trying to access directoryEntries, if the directory doesn't exist. In particular it will throw an exception ex for which the following property holds: ex.result == Cr.NS_ERROR_FILE_TARGET_DOES_NOT_EXIST You would have to add an appropriate exception handling mechanism to ensure that the method terminates gracefully even when the directory does not exist. ::: browser/components/dirprovider/DirectoryProvider.cpp @@ +95,5 @@ > nsresult rv = aDirSvc->Get(key, NS_GET_IID(nsIFile), getter_AddRefs(file)); > if (NS_FAILED(rv)) > return; > > + if (NS_FAILED(rv)) There is no need for this test since you removed the call to exists. @@ +130,5 @@ > return; > searchPlugins->SetNativeLeafName(NS_LITERAL_CSTRING("distribution")); > searchPlugins->AppendNative(NS_LITERAL_CSTRING("searchplugins")); > > + if (NS_FAILED(rv)) Same here @@ +282,5 @@ > mNext->AppendNative(nsDependentCString(*i)); > ++i; > } > > + if (NS_SUCCEEDED(rv)) Same here.
Attachment #8477891 - Flags: review-
hello, is somebody working on this bug because i would like to work on this bug, this would be my first bug so i might need some extra guidance debugging it. thanks!
Diwas, Anuj is already working on it.
hi, if someone's working on it, please change the assigned to from no one to that person Thanks
Assignee: nobody → kamikazeanuj
Flags: qe-verify?
Anuj, are you still working on this?
Flags: needinfo?(kamikazeanuj)
Hi Roberto, Ya, I'm working on this bug. Let it be assigned to me.
Flags: needinfo?(kamikazeanuj)
Flags: qe-verify? → qe-verify-
Just wondering if this is still being worked on.
Assignee: kamikazeanuj → nobody
Craig, feel free to work on it.
Will do.
Assignee: nobody → parkcra
I have implemented the changes, including corresponding error handling. The source builds and runs fine, however it doesn't pass the following xpcshell tests :- /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/js/xpconnect/tests/unit/test_file.js ------------------------------------------------------------------------------------------------------------ FAIL [Parent] /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/js/xpconnect/tests/unit/test_file2.js ------------------------------------------------------------------------------------------------------------- FAIL [Parent] /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/netwerk/test/unit/test_udp_multicast.js --------------------------------------------------------------------------------------------------------------- FAIL [Parent] /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/security/manager/ssl/tests/unit/test_pkcs11_insert_remove.js ------------------------------------------------------------------------------------------------------------------------------------ TIMEOUT [Parent] Can your provide some insight into the tests and how I can investigate them. Thanks.
(In reply to Craig Parkinson from comment #21) > I have implemented the changes, including corresponding error handling. Great! > The source builds and runs fine, however it doesn't pass the following xpcshell > tests :- > > /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/js/ > xpconnect/tests/unit/test_file.js > ----------------------------------------------------------------------------- > ------------------------------- > FAIL [Parent] > /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/js/ > xpconnect/tests/unit/test_file2.js > ----------------------------------------------------------------------------- > -------------------------------- > FAIL [Parent] > /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/ > netwerk/test/unit/test_udp_multicast.js > ----------------------------------------------------------------------------- > ---------------------------------- > FAIL [Parent] > /home/craig/src/mozilla/mozilla-central/objdir-ff-debug/_tests/xpcshell/ > security/manager/ssl/tests/unit/test_pkcs11_insert_remove.js > ----------------------------------------------------------------------------- > ------------------------------------------------------- > TIMEOUT [Parent] > > Can your provide some insight into the tests and how I can investigate them. > Thanks. You can run a xpcshell test individually to see exactly where it fails, e.g.: ./mach xpcshell-test security/manager/ssl/tests/unit/test_pkcs11_insert_remove.js
Attached patch proposed fix - bug-1003968.diff (obsolete) (deleted) — — Splinter Review
Proposed patch attached, from what I can tell the failed xpcshell tests already failed in the original source tree. Appreciate any comments on the proposed patch.
Attachment #8533975 - Flags: review?(rvitillo)
Comment on attachment 8533975 [details] [diff] [review] proposed fix - bug-1003968.diff Review of attachment 8533975 [details] [diff] [review]: ----------------------------------------------------------------- Please use as description for your patch the same one used in the Bug Report. When a non-existing directory is accessed in the sync path at http://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?from=nsSearchService.js#3153 a NS_ERROR_FILE_TARGET_DOES_NOT_EXIST exception is thrown which you are not handling. You might find useful to temporarily disable the async path by e.g. commenting out http://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?from=nsSearchService.js#3911 to ensure that the sync path is handled correctly. ::: toolkit/components/search/nsSearchService.js @@ +3239,1 @@ > } Please wrap in a try block just the minimum amount of code necessary. In this case it's not even needed as the exception is raised by iterator.next() and is catched by the enclosing try block. You should add a comment in the existing catch block that says that we are also catching exceptions for non-existing directories. @@ +3518,3 @@ > } > + } catch(ex) { > + //catch FileTargetException Please wrap in a try block just the minimum amount of code necessary. @@ +3571,5 @@ > + }.bind(this)); > + } catch (ex) { > + var promise = new Promise(); > + return promise.reject();//do not load if aDir doesn't exist > + } Same here.
Attachment #8533975 - Flags: review?(rvitillo) → review-
When returning a rejected promise from a caught exception at http://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?from=nsSearchService.js#3528, when no search plugin directory exists, the default search engine is now null. This causes set_defaultEngine() to now fall over (it checks the instanceof the value passed to it) http://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js?from=nsSearchService.js#4218. How do we think this should be handled, as get_DefaultEngine and set_DefaultEngine now have invalid types.
_asyncLoadEnginesFromDir returns an array of engines for a directory. If no engines are available in a directory, because e.g. the directory doesn't exist, it should return an empty array.
Attached patch Proposed patch (obsolete) (deleted) — — Splinter Review
Proposed fix, containing the requested changes. I tested the syncronous entry path and that works.
Attachment #8533975 - Attachment is obsolete: true
Attachment #8537111 - Flags: review?(rvitillo)
Comment on attachment 8537111 [details] [diff] [review] Proposed patch Review of attachment 8537111 [details] [diff] [review]: ----------------------------------------------------------------- The patch can't be applied cleanly, i.e. it's based on your last rejected patch.
Attachment #8537111 - Flags: review?(rvitillo) → review-
Attached patch clean patch (obsolete) (deleted) — — Splinter Review
Clean Patch
Attachment #8537111 - Attachment is obsolete: true
Attachment #8537788 - Flags: review?(rvitillo)
Comment on attachment 8537788 [details] [diff] [review] clean patch Review of attachment 8537788 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/dirprovider/DirectoryProvider.cpp @@ +91,5 @@ > AppendFileKey(const char *key, nsIProperties* aDirSvc, > nsCOMArray<nsIFile> &array) > { > nsCOMPtr<nsIFile> file; > + aDirSvc->Get(key, NS_GET_IID(nsIFile), getter_AddRefs(file)); We should keep the test on the returned result. ::: toolkit/components/search/nsSearchService.js @@ +3154,5 @@ > + if (dir.directoryEntries.hasMoreElements()) > + loadDirs.push(dir); > + } catch (ex) { > + // catch directory doesn't exist > + } As we are catching a particular exception here, we should remove the comment and be explicit about the exception we are catching using a conditional catch clause, e.g. catch(ex if ex.result == ...). @@ +3481,5 @@ > }, > > _loadEnginesFromDir: function SRCH_SVC__loadEnginesFromDir(aDir) { > LOG("_loadEnginesFromDir: Searching in " + aDir.path + " for search engines."); > + Don't add whitespace to empty lines. @@ +3488,5 @@ > + var isInProfile = aDir.equals(getDir(NS_APP_USER_SEARCH_DIR)); > + } catch(ex) { > + return; > + // catch Target not exits exception > + } We should use a conditional catch clause in this case. @@ +3489,5 @@ > + } catch(ex) { > + return; > + // catch Target not exits exception > + } > + Don't add whitespace to empty lines. @@ +3494,3 @@ > var files = aDir.directoryEntries > + .QueryInterface(Ci.nsIDirectoryEnumerator); > + Same here. @@ +3537,5 @@ > + try { > + // Check whether aDir is the user profile dir > + let isInProfile = aDir.equals(getDir(NS_APP_USER_SEARCH_DIR)); > + } catch (ex) { > + //do not load if aDir doesn't exist Please use a conditional catch clause. @@ +3542,2 @@ > let engines = []; > + return engines; "return [];" is sufficient. @@ +3792,5 @@ > for each (engine in this._engines) { > var orderNumber = engineMetadataService.getAttr(engine, "order"); > > // Since the DB isn't regularly cleared, and engine files may disappear > + // without us knowing, we may already have an engine in this slot. If.QueryInterface(Ci.nsIDirectoryEnumerator); ?
Attachment #8537788 - Flags: review?(rvitillo) → review-
Attached patch 1003968v2.diff (deleted) — — Splinter Review
implemented requested changes.
Attachment #8537788 - Attachment is obsolete: true
Attachment #8539842 - Flags: review?(rvitillo)
Comment on attachment 8539842 [details] [diff] [review] 1003968v2.diff Review of attachment 8539842 [details] [diff] [review]: ----------------------------------------------------------------- There are some mochitest and xpcshell-test failures, please have a look at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cb9ec13467e (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d0c9f5edb1c without your patch).
Attachment #8539842 - Flags: review?(rvitillo) → review-
In investing one of the xpcshell-test failures, one of the failed tests checks the value of the selected engine. However with the removal of the exists() calls it is now possible for Services.Search.currentEngine to be null, which is causing this particular test to fall over. Any advice on how to deal with this? another try catch with the exception returning a string?
Flags: needinfo?(rvitillo)
(In reply to Craig Parkinson from comment #33) > In investing one of the xpcshell-test failures, one of the failed tests > checks the value of the selected engine. Which test? > However with the removal of the exists() calls it is now possible for Services.Search.currentEngine to be > null, which is causing this particular test to fall over. Any advice on how > to deal with this? another try catch with the exception returning a string? Not sure I understand what you mean with "another try catch", could you show me a concrete example? Thanks.
Flags: needinfo?(rvitillo)
>Which test? toolkit/components/search/tests/xpcshell/test_selectedEngine.js The result of test shows that Services.Search.currentEngine is null, which suggest that no engine is returned from the search, even if those engines exist. Am I correct in this assumption? >Not sure I understand what you mean with "another try catch", could you show me a concrete example? It was an hypothetical question rather than concrete. The value of Services.Search.currentEngine must be of a value (not null), however it is now possible for it to be null if no engines exist, unless the above case is true (in that the search doesn't return engines).
Flags: needinfo?(rvitillo)
(In reply to Craig Parkinson from comment #35) > >Which test? > > toolkit/components/search/tests/xpcshell/test_selectedEngine.js > > The result of test shows that Services.Search.currentEngine is null, which > suggest that no engine is returned from the search, even if those engines > exist. Am I correct in this assumption? No. If the test was returning an engine before your patch and it doesn't after it, then something is wrong. The test triggers an asynchronous re-initilization of the search service and checks for the currentEngine only after that initilization has completed.
Flags: needinfo?(rvitillo)
Hi, I am a beginner and I was wondering if anyone is still working on this ticket? If possible, I would like to take a shot. Can anyone assign it to me?
Feel free to work on this, I unfortunately do not have the time to commit to it.
Mentor: rvitillo
Despite all the search service refactorings of the last couple years, this bug surprisingly still exists, at least for the AppendDistroSearchDirs case, and seems to do startup main thread IO in the *async* init code path of the search service. I'm surprised we don't check the distribution.id pref before running the code at https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/toolkit/components/search/nsSearchService.js#2958
Blocks: photon-startup
Priority: -- → P2
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][fx-search]
Whiteboard: [good first bug][lang=c++][fx-search] → [good first bug][lang=c++][fxsearch]
Whiteboard: [good first bug][lang=c++][fxsearch] → [good first bug][lang=c++][fxsearch] [fxperf]
Whiteboard: [good first bug][lang=c++][fxsearch] [fxperf] → [good first bug][lang=c++][fxsearch] [fxperf:p2]
Assignee: parkcra → nobody
Hello, I was trying to work on this bug and I am unable to find the method AppendFileKey in browser/components/dirprovider/DirectoryProvider.cpp as mentioned in the comments and also in the patches attached here. Could someone help me find the method AppendFileKey?
AppendFileKey doesn't seem to exist anymore, but AppendDistroSearchDirs still exists.
AppendDistroSearchDirs and DirectoryProvider::AppendingEnumerator::GetNext in browser/components/dirprovider/DirectoryProvider.cpp still calls Exists(bool var); But it seems like both these methods are not being used anywhere else? Should I remove it?

Florian, what's the current state here? Is this still current, should it still be a good-first-bug, and is the link in comment #44 a good place to start or no?

Flags: needinfo?(florian)

(In reply to :Gijs (he/him) from comment #45)

Florian, what's the current state here? Is this still current

Definitely still exists. https://i.imgur.com/GsXHqkS.png shows the stack in today's nightly.

should it still be a good-first-bug,

I doubt it without a mentor.

and is the link in comment #44 a good place to start or no?

A generic link to the search service isn't a good place to start, no.

Mike, is there anybody in your team who could mentor this?

Flags: needinfo?(florian) → needinfo?(mdeboer)

I guess I can mentor this bug. If the C++ becomes too complicated for me, I'll defer to mak.

Mentor: mdeboer, mak77
Flags: needinfo?(mdeboer)

Hi
I would like to contribute to mozilla . If this is not assigned to anyone i would like work on it. This will be my first bug and any help or pointers will be really appreciated.

I would also like to try this as my first bug!

Mike, can you help people get started here?

Flags: needinfo?(mdeboer)

Ruthra has just reached out to me through email and as soon as there's a patch attached here I can help the assignee out with any problems they might stumble upon!

Flags: needinfo?(mdeboer)

Hi,
sorry about my last comment. Contents of this page was in my clipboard and accidentally got pasted here. please ignore my previous comment.

Removed Exists() call from DirectoryProvider.cpp. some 31 xpcshell-tests are failing after the change. 27 are from security/manager/ssl/tests/unit directory. Most of them fail at this line https://searchfox.org/mozilla-central/source/security/manager/ssl/tests/unit/head_psm.js#502 with the following error.

Please check the patch file "1003968_u1.diff" attached.

============================================================================================================================

0:02.60 ERROR ReferenceError: Utilbin is not defined at c:/Users/lenovo/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/_tests/xpcshell/security/manager/ssl/tests/unit/head_psm.js:501
_getBinaryUtil@c:/Users/lenovo/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/_tests/xpcshell/security/manager/ssl/tests/unit/head_psm.js:501:5
_setupTLSServerTest@c:/Users/lenovo/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/_tests/xpcshell/security/manager/ssl/tests/unit/head_psm.js:542:19
add_tls_server_setup/<@c:/Users/lenovo/mozilla-source/mozilla-central/obj-x86_64-pc-mingw32/_tests/xpcshell/security/manager/ssl/tests/unit/head_psm.js:358:5
_run_next_test@c:\Users\lenovo\mozilla-source\mozilla-central\testing\xpcshell\head.js:1453:11
run@c:\Users\lenovo\mozilla-source\mozilla-central\testing\xpcshell\head.js:686:9
_do_main@c:\Users\lenovo\mozilla-source\mozilla-central\testing\xpcshell\head.js:224:6
_execute_test@c:\Users\lenovo\mozilla-source\mozilla-central\testing\xpcshell\head.js:527:5
@-e:1:1

============================================================================================================================

It looks like this bug has stalled out. A patch was posted in comment 52, but it doesn't look like ruthra ever received any feedback - at least, not here in Bugzilla.

ruthra, are you still interested in working on this if I can find someone to give you feedback on it?

Flags: needinfo?(ruthrab)

I'm definitely interested. Removed the exists() calls as mentioned but its causing couple of test cases to fail, not sure how to proceed from there. Any feedback or guidance will be really appreciated.

Flags: needinfo?(ruthrab)

Mike, who did you have in mind for feedback here? :-)

Flags: needinfo?(mconley)

For giving feedback, I think I can do it, but might pull in a daleharvey or Standard8 for some searchy-bits.

I've started to piece together what we're doing in this file, and your approach looks great, ruthra! I ran the tests locally, and I wasn't able to reproduce the test failures you were hitting in the security/manager/ssl/tests/ directory.

Instead, what I got was failures initializing the SearchService, but I think I've got an idea of how to fix that by telling the SearchService to not freak out if the directories it's trying to access don't exist.

I have a try push cooking here to run the tests in automation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=113967b72b8e3ee3f1392ff0b7ea95f3bafe5f91

Keeping the needinfo on myself to look back on this when the try push has had time to run the tests.

Flags: needinfo?(mdeboer)

Hey ruthra, so looking at the try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=113967b72b8e3ee3f1392ff0b7ea95f3bafe5f91, I believe those failures are all known intermittents, so it looks all green!

The only thing I added in addition to your patch was this change to SearchServices.jsm: https://hg.mozilla.org/try/rev/6f430c8a70d1cccb2992695a6a962643d3d47e44

What that does is prepare the SearchService for the possibility that the DirectoryIterator is going to report back that some directories don't exist (which is expected - that's what your patch allows us to do).

Do you think you could rebase your patch against the most recent tip of mozilla-central, and fold my change in? Then we should get it up for review with Phabricator, setting myself and maybe daleharvey as reviewers. I can walk you through any part of that, just let me know when you get stuck, okay?

Assignee: nobody → ruthrab
Mentor: mak77, mdeboer → mconley
Flags: needinfo?(mconley) → needinfo?(ruthrab)

This Change removes all call to Exists() in Directory Provider component, which creates the possibility for the componenet to return an empty list. SearchService.jsm is modified to handle this possibility.

hi mike. Changes are made on latest tip and have been uploaded in Phabricator. Requesting for a review.

Flags: needinfo?(ruthrab) → needinfo?(mconley)

Great! Can you fix the formatting issues flagged up by eslint, and the notes I made on the C++ portion of the patch? Thank you!

Flags: needinfo?(mconley) → needinfo?(ruthrab)

This Change removes all call to Exists() in Directory Provider component, which creates the possibility for the componenet to return an empty list. SearchService.jsm is modified to handle this possibility.

Hi Gijs, corrections are made as per review and i've removed the section pointed by florian as well. Kindly check.

Flags: needinfo?(ruthrab)
Component: General → Search
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/de151ad69bd6 avoid Exists() calls for search plugin directories in the browser directory provider r=daleharvey,Gijs

(In reply to Dorel Luca [:dluca] from comment #66)

Backed out changeset de151ad69bd6 (bug 1003968) for build bustage. CLOSED TREE

Specifically:

[task 2019-08-21T09:48:24.516Z] 09:48:24    ERROR -  browser/components/dirprovider/DirectoryProvider.cpp:137:12:
                  error: unused variable 'rv' [-Werror,-Wunused-variable]
[task 2019-08-21T09:48:24.516Z] 09:48:24     INFO -    nsresult rv;
[task 2019-08-21T09:48:24.516Z] 09:48:24     INFO -             ^
[task 2019-08-21T09:48:24.516Z] 09:48:24     INFO -  1 error generated.

Just removing that line should do. I've commented on phabricator as well. :-)

removed the unused variable and updated phab revision D42772

Flags: needinfo?(ruthrab)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/6162c3a1afb5 avoid Exists() calls for search plugin directories in the browser directory provider r=daleharvey,Gijs

https://hg.mozilla.org/mozilla-central/rev/6162c3a1afb5

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: