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

NEW
Unassigned

Status

()

defect
P2
normal
5 years ago
2 months ago

People

(Reporter: Gavin, Unassigned, Mentored, NeedInfo)

Tracking

(Blocks 2 bugs, {main-thread-io, perf})

29 Branch
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++][fxsearch] [fxperf:p2])

Attachments

(3 attachments, 3 obsolete attachments)

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++]

Comment 1

5 years ago
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++]

Comment 5

5 years ago
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)

Comment 7

5 years ago
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.

Comment 9

5 years ago
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.

Comment 11

5 years ago
Posted patch 1003968.diffSplinter 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.

Comment 15

5 years ago
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)

Comment 17

5 years ago
Hi Roberto,

Ya, I'm working on this bug. Let it be assigned to me.
Flags: needinfo?(kamikazeanuj)
Flags: qe-verify? → qe-verify-

Comment 18

5 years ago
Just wondering if this is still being worked on.
Assignee: kamikazeanuj → nobody
Craig, feel free to work on it.

Comment 20

5 years ago
Will do.
Assignee: nobody → parkcra

Comment 21

5 years ago
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

Comment 23

5 years ago
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-

Comment 25

5 years ago
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.

Comment 27

5 years ago
Posted patch Proposed patch (obsolete) — 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-

Comment 29

5 years ago
Posted patch clean patch (obsolete) — 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-

Comment 31

5 years ago
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-

Comment 33

4 years ago
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)

Comment 35

4 years ago
>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)

Comment 37

3 years ago
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?

Comment 38

3 years ago
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
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

Comment 40

11 months ago
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.

Comment 42

11 months ago
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?

Comment 45

5 months ago

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)

Comment 48

4 months ago

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.

Comment 49

3 months ago

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

Comment 50

3 months ago

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)
Comment hidden (typo)

Comment 53

3 months ago

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

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

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