727555 - Refactor nsSearchService to use Map

Status: RESOLVED WONTFIX

(Firefox :: Search, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

Followup to bug 699856. The engine metadata service is a pure JavaScript object. This can be unsafe if some day we decide to make use of keys whose name match properties/methods of Object.

We should use Dict.jsm to ensure that this does not happen.

Comment 1

[Abhishek Potnis [:avp]]

Hi Yoric,
I am interested in working on this bug. I am a beginner, so could you please help me on getting started on this bug.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Hi Abhishek,

As discussed on IRC, we will start with bug 727967.

Comment 3

[Abhishek Potnis [:avp]]

sure

Comment 4

[Abhishek Potnis [:avp]]

Hi Yoric,
Could you please guide me on getting started with this bug.

Thanks.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

This one is a little more complex than the parent bug.
The objective is to consolidate/simplify a little the code of nsSearchService.js .

This code uses a small file to store a few informations about the search bar (e.g. in which order the user wants to see the engines). In a previous bug, we have changed the code to ensure that it did not use a full-fledged database, which was overkill and slowed things down unnecessarily, but rather a simple JSON file.

Right now, nsSearchService.js reimplements a subset of Dict.jsm. The idea is to instead reuse Dict.jsm.

So, your objective is to ensure that:
- function |_loadStore| does not return a raw object but rather a dictionary (you will need to use your new constructor for dictionaries);
- the methods that use the store (|getAttr| and |_setAttr|) use it correctly, as a dictionary.

Currently, |_loadStore| is at: http://dxr.lanedo.com/mozilla-central/toolkit/components/search/nsSearchService.js.html#l3639

Comment 6

[Abhishek Potnis [:avp]]

Hi Yoric,
Shall I edit _migrateOldDB: function SRCH_SVC_EMS_migrate() ? coz here the store object is created and values are set.......I could create a dictionary here only and then simply return it to _loadStore, but will it affect the other code ? please help....

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Indeed, you will also need to edit |_migrateOldDB| to create a dictionary.

Comment 8

[Abhishek Potnis [:avp]]

Hi Yoric,
Should I create a dictionary with engine, name and values as the keys ?
Please assign this bug to me, as I have started working on it


Thanks.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Good question. I think you can try with just engine name as the key and an object as a value. I believe it should work.

By the way, you should modify your Bugzilla name so that it contains your IRC nickname (just like mine contains [:Yoric]) . This will make it easier for people to find you on IRC.

Comment 10

[Abhishek Potnis [:avp]]

Attachment: edited |_migrateOldDB|

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 639656 [details] [diff] [review]
edited  |_migrateOldDB|

Review of attachment 639656 [details] [diff] [review]:
-----------------------------------------------------------------

Either I misunderstand your code or there is an error. Could you please check and/or explain?

Also, I have come to the conclusion that the suggestion I gave you over IRC is not optimal. If, as I suggested, you use your directory to associate engine_name -> object_with_all_engine_properties, your object_with_all_engine_properties can cause issues if any property name ends up being a reserved field name, such as "__proto__".

So, what you should do, rather, is the following:
- instead of using |engine_name| as keys, prefix all engine names with |@| in |_setAttr|/|getAttr|/|_migrateOldDB| (this will avoid collisions with reserved field names);
- use an object to associate @engine_name -> dictionary_of_all_object_properties.

Again, you probably only need to change methods |_setAttr|, |getAttr|, |_loadStore| and |_migrateOldDB|.

::: toolkit/components/search/nsSearchService.js
@@ +3760,5 @@
>        LOG("SRCH_SVC_EMS_migrate Migrating data from SQL");
>        const sqliteDb = Services.storage.openDatabase(sqliteFile);
>        const statement = sqliteDb.createStatement("SELECT * from engine_data");
> +      let someObj={};
> +      let SDict = new Dict({eng_name: someObj});

Not sure I understand. What is |eng_name|?
If I understand correctly your intention, the dictionary should be created empty.

@@ +3768,5 @@
>          let name   = row.name;
>          let value  = row.value;
> +	someObj.engine = engine;
> +	someObj.value = value;
> +	SDict.set(name,someObj);

This looks fishy.
You are sharing one object for the whole loop, this is bound to hurt.

Comment 12

[Abhishek Potnis [:avp]]

Attachment: made the suggested changes in |_migrateOldDB|

I have re-edited |_migrateOldDB|. I have prefixed engine names with |@| and used the object store with engine_name ie |name| as the index and stored the Dictionary my Dict in the store[name]. In the while loop, each time it iterates a new Dictionary will be created and stored in corresponding store[name].
Is this part correct ?

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 642337 [details] [diff] [review]
made the suggested changes in  |_migrateOldDB|

Review of attachment 642337 [details] [diff] [review]:
-----------------------------------------------------------------

I'm afraid that this is not really how dictionaries work. Let me suggest the following: first, take a look at the file search-metadata.json in your Firefox profile. This file contains the value of |store|, so this may give you a better idea of what |engine|, |value|, etc. work.

You should make sure that you understand this before writing code for this bug. Also, it is probably a good idea to make sure that you understand all the functions of |engineMetadataService|.

Comment 14

[Abhishek Potnis [:avp]]

Hi David,
I took a look at the file search-metadata.json, but I am sorry I could not make out anything from it. Here is what I found there :    

"[app]/amazondotcom.xml": {
        "used": 0,
        "order": 4
    },
    "[app]/eBay.xml": {
        "used": 0,
        "order": 1
    },
    "[app]/wikipedia.xml": {
        "used": 0,
        "order": 6
    },
    "[app]/twitter.xml": {
        "used": 0,
        "order": 7
    },
    "[app]/google.xml": {
        "used": 1,
        "order": 2
    },
    "[app]/bing.xml": {
        "order": 5
    },
    "[app]/yahoo.xml": {
        "order": 3
    }
}

Could you please help me with this ?

I went through the functions in |engineMetadataService| and have fairly understood them. Please help me on this bug as to how should I proceed with it.

Thanks.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

In this file, "[app]/amazondotcom.xml" (and all others of the same kind) are engine IDs. "used" and "order" are attributes, which can be accessed with getAttr/set with setAttr.

Comment 16

[Abhishek Potnis [:avp]]

Attachment: edited |_migrateOldDB|, |getAttr| and |_setAttr| to use Dictionary

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Note: JS now has a native Map(), which obsoletes Dict.jsm for most simple use cases. I think this could just use Map() instead.

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Map

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 650928 [details] [diff] [review]
edited  |_migrateOldDB|, |getAttr| and |_setAttr| to use Dictionary

Review of attachment 650928 [details] [diff] [review]:
-----------------------------------------------------------------

This is a start. However, I believe that this patch does not work, and it is also incomplete. As I explained over IRC, I want |_store| to be a dictionary of dictionaries. Also, you will need to adapt both loading and saving of |_store|.

::: toolkit/components/search/nsSearchService.js
@@ +3680,5 @@
>    _setAttr: function epsSetAttr(engine, name, value) {
>      // attr names must be lower case
>      name = name.toLowerCase();
>      let db = this._store;
>      let record = db[engine._id];

For the moment, let's try and rewrite |this._store| as a Dictionary.

@@ +3769,3 @@
>          let value  = row.value;
> +	
> +        if (!store[engineid]) {

Here, too, let's make |store| a dictionary. We will see if we can optimize this with these "@" + name stuff at a later stage.

@@ +3769,5 @@
>          let value  = row.value;
> +	
> +        if (!store[engineid]) {
> +          store[engineid] = {};
> +	  var myDict = new Dict();

You should use |let| rather than |var|...

@@ +3774,3 @@
>          }
> +	else{
> +	let myDict=store[engineid];

... however, that is not how |let| works.

Comment 19

[Abhishek Potnis [:avp]]

Hi Yoric,

I have understood that store must be a dictionary of dictionaries. So shall I make it as engineid as the keys and dictionaries (that I have currently used) as values ? Also I am confused about correcting the last mistake pointed out by you - 
 
/**
@@ +3774,3 @@
>          }
> +	else{
> +	let myDict=store[engineid];

... however, that is not how |let| works.
**/

Here I want the dictionary that will be returned by |store[engineid]| to be caught in a Dictionary object. Should I make a empty Dictionary like 
let tempDict = new Dict();
and then use the copy method of Dict.jsm to copy the contents returned by |store[engineid]| into |tempDict| ? Or is there some other way to do it in less lines of code ?

Please help me on this.

Thanks.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

(In reply to Abhishek Potnis [:abhishekp] from comment #19)
> Hi Yoric,
> 
> I have understood that store must be a dictionary of dictionaries. So shall
> I make it as engineid as the keys and dictionaries (that I have currently
> used) as values ?

That's the idea, indeed.

> Also I am confused about correcting the last mistake
> pointed out by you - 
>  
> /**
> @@ +3774,3 @@
> >          }
> > +	else{
> > +	let myDict=store[engineid];
> 
> ... however, that is not how |let| works.
> **/

I am pointing out that this use of |let| is incorrect.

|let| will define a variable that is local to the block, so this definition of |myDict| is, normally, not valid outside of your |else { ... }|. Now, you have also defined |var myDict| a few lines above, which defines a variable that is local to the function, including the |else { ... }|. This is an error.

Using |let| is the right thing to do, but you should place that |let| in the right position.

> 
> Here I want the dictionary that will be returned by |store[engineid]| to be
> caught in a Dictionary object. Should I make a empty Dictionary like 
> let tempDict = new Dict();
> and then use the copy method of Dict.jsm to copy the contents returned by
> |store[engineid]| into |tempDict| ? Or is there some other way to do it in
> less lines of code ?

No, do not copy the dictionary. You should:
- define variable |myDict|;
- check if the store (the dictionary that maps |engineid| to dictionaries) already has a dictionary for key |engineid|;
- if this dictionary exist, set |myDict| to this dictionary;
- otherwise, create that dictionary, set |myDict| to the new dictionary and add it to the store for key |engineid|;
- then perform that |myDict.set(name, value)|.

This is almost the pattern that you are replacing in |_migrateOldDB|. You just need to adapt it to dictionaries instead of raw objects.

> 
> Please help me on this.
> 
> Thanks.

Comment 21

[Abhishek Potnis [:avp]]

Attachment: made the suggested changes in |_migrateOldDB|,|getAttr| and |setAttr|

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 653252 [details] [diff] [review]
made the suggested changes in  |_migrateOldDB|,|getAttr| and |setAttr|

Review of attachment 653252 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, this time, it is a good start. Now, let's proceed to completing the patch :)

Also, please fix the following items.

::: toolkit/components/search/nsSearchService.js
@@ +3685,2 @@
>      if (!record) {
> +      record = {};

Do not forget that records are dictionaries, not arbitrary empty objects.

@@ +3767,2 @@
>          let name   = row.name;
> +	name = "@"+name;

As mentioned previously, let's forget about this "@" for the moment.

@@ +3769,2 @@
>          let value  = row.value;
> +	let myDict = {};

Why an empty object here? Especially since you give a value to |myDict| in both branches of the following |if|/|else|.

Also, to harmonize stuff, instead of calling the dictionary |myDict|, I suggest calling it |record|, as in |getAttr| and |_setAttr|.

@@ +3774,5 @@
> +	else{
> +	myDict=store.get(engineid);
> +	}
> +	myDict.set(name,value);
> +        store.set(engineid,myDict);

You only need to do this |store.set| operations if |myDict| is not in |store| yet.

Comment 23

[Abhishek Potnis [:avp]]

Attachment: made the suggested changes in |_migrateOldDB|,|getAttr| and |setAttr|

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 653270 [details] [diff] [review]
made the suggested changes in  |_migrateOldDB|,|getAttr| and |setAttr|

Review of attachment 653270 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Now, time for the rest of the work.

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Per comment 17, I don't think we should be adding any new Dict.jsm users that could be using Map() instead. Is that the case here?

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Sorry, we missed #comment 17.

I was actually suspecting that we would want to port Dict.jsm (which now supports JSON) on top of Map() (which doesn't yet). Of course, we can also ditch Dict.jsm for Map() immediately. Your call, gavin.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

If Map() is sufficient, let's use Map().

Comment 28

[Abhishek Potnis [:avp]]

I have read the documentation of Map....it is pretty much similar to Dictionary so wont be having difficulty using it......Should I start with converting my previous patch in which I had used Dictionary to use Map ? Also if we are doing this then we may need to edit this bug's title........

Comment 29

[David Teller [:Yoric] - still alive but not very active]

(In reply to Abhishek Potnis [:abhishekp] from comment #28)
> I have read the documentation of Map....it is pretty much similar to
> Dictionary so wont be having difficulty using it......Should I start with
> converting my previous patch in which I had used Dictionary to use Map ?
> Also if we are doing this then we may need to edit this bug's title........

Yes, if you could convert your previous patch to use Map, this would be a start. Of course, don't forget to add the features, i.e. updating the methods that load from JSON/save to JSON.

Comment 30

[Abhishek Potnis [:avp]]

Attachment: edited|_migrateOldDB|,|getAttr| and |setAttr| to use Map

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 655948 [details] [diff] [review]
edited|_migrateOldDB|,|getAttr| and |setAttr| to use Map

Review of attachment 655948 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, with the minor layout changes mentioned below (you don't need to f? me for these changes).
However, this patch is just as incomplete as the previous ones. Could you please add the missing features? i.e. loading the store from JSON, saving it back to JSON.

::: toolkit/components/search/nsSearchService.js
@@ +3772,3 @@
>          }
> +	else{
> +	record=store.get(engineid);

Don't forget to match the layout of the rest of the patch: whitespace between |else| and |{|, two more spaces on the next line.

@@ +3772,5 @@
>          }
> +	else{
> +	record=store.get(engineid);
> +        }
> +	record.set(name,value);   

Don't forget to remove the whitespace you have added at the end of the line.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

At this stage, we have a problem. To serialize a map to JSON, we need to extract its set of keys. This is currently not implemented in |Map|. We could work around this, by storing the set of keys in a field |keys|, but this seems overkill.

Gavin, any thought about that?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Maps are iterable, per bug 725909:
let m = new Map();
m.set("1", "2");
for (let [a,b] of m)
  print(a + ":" + b);

produces "1:2".

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Gavin: Good to know – we should probably fix the MDN documentation.

Abhishek: This should give you a base to build a JavaScript object that you can convert to JSON:

- create an empty JavaScript object |foo| that will serve as a JSON holder for |_store|;
- for each key/value pair of |_store|, do
  - create a new JS object |bar| that will serve as a JSON holder for the value;
  - associate |bar| to |foo[":" + key]|
  - for each key2/value2 pair in the value, do
     - |bar[":" + key2] = value|
- then convert |foo| to JSON with |JSON.stringify(foo)|.

Of course, you should find better names than |foo| and |bar|.

Comment 35

[Abhishek Potnis [:avp]]

Attachment: edited |_migrateOldDB|,|getAttr|,|setAttr| and |_commit| to use Map

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 656832 [details] [diff] [review]
edited |_migrateOldDB|,|getAttr|,|setAttr| and |_commit| to use Map

Review of attachment 656832 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Now you need to implement loading and you will be able to test.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #34)
> Gavin: Good to know – we should probably fix the MDN documentation.

Bug 725909 is marked dev-doc-needed :)

Comment 38

[Abhishek Potnis [:avp]]

Hi Yoric,

Could you please show me as to how I must proceed to load the Map from JSON ?


Thanks.

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Well:
- read the JSON file (that's done already in function epsInit);
- parse it using JSON.parse (as done in function epsInit);
- this gives you an object that contains a set of keys and values;
- create an empty Map;
- iterate through the keys and values of that object and copy them into the Map.

Comment 40

[Abhishek Potnis [:avp]]

Attachment: edited |_asyncMigrateOldDB|,|_syncMigrateOldDB|,|getAttr| and |_setAttr| functions

The nsSearchService.js file has changed a lot since I edited it long back. There are two functions named migrateOldDB (Asynch and Synch), so I have made the changes in both. I have also edited getAttr and _setAttr functions. But now I am not understanding how I must implement this :https://bugzilla.mozilla.org/show_bug.cgi?id=727555#c34, in the |_commit|function.
Please help me here.

Thanks.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 701526 [details] [diff] [review]
edited |_asyncMigrateOldDB|,|_syncMigrateOldDB|,|getAttr| and |_setAttr| functions

Review of attachment 701526 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/nsSearchService.js
@@ +3829,2 @@
>        return null;
> +    return record.get(aName);

This can be shortened, since |record.get| returns |null| when the key is not defined.

@@ +3919,2 @@
>           }
> +         else {

Nit: } else {

@@ +3919,3 @@
>           }
> +         else {
> +	   record = store.get(engineid);	 

Nit: Please remove these trailing whitespaces.

@@ +3970,2 @@
>                 }
> +               else {

Nit: Here, too } else {.

@@ +3970,3 @@
>                 }
> +               else {
> +		 record = store.get(engineid);			

Nit: Here, too, remove the trailing spaces.

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Any news, abhishekp?

Comment 43

[Abhishek Potnis [:avp]]

Attachment: WIP

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 722251 [details] [diff] [review]
WIP

Review of attachment 722251 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. You still need to implement parsing, though.

Comment 45

[David Teller [:Yoric] - still alive but not very active]

Any news?

Comment 46

[Abhishek Potnis [:avp]]

Attachment: WIP

I have loaded from the json into the map in the epsInit() function. If these changes are correct, then I will proceed with the epsSyncInit() function.

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 742331 [details] [diff] [review]
WIP

Review of attachment 742331 [details] [diff] [review]:
-----------------------------------------------------------------

Let me suggest another strategy: finish the work, see if it passes the tests, and then we will discuss and determine if the code needs improvements.

Comment 48

[Abhishek Potnis [:avp]]

Attachment: Refractored nsSearchService to use Map

The nsSearchService.js had changed, so my previous patch was not being applied to it. I have made the changes and now the patch applies. I am attaching the latest patch. I built the src and tested and all the tests failed :(

I have pastebined the test results: http://pastebin.mozilla.org/2497598

I went through the test results and this is the error that repeats "error running onStopped callback: TypeError: callback is not a function". I am not understanding what needs to be done(from this error). Can you please guide me to get started with the debugging to check where my code has errors? Also should the tests need to be edited?

Please help me on this.

Thanks.

Comment 49

[Abhishek Potnis [:avp]]

Hi Yoric,

I had forgotten about this bug completely, until today when I checked my Bugzilla Dashboard. I have been busy, not being able to devote any time for bug-fixing recently.
I guess I wont be able to devote much time for the next couple of months too, I could try some easy ones for now, and this does not happen to fall under that category, not with the file having changed a lot over the course of time.

Am un-assigning myself, so others can feel free to take it.

Thanks.

Comment 50

[Abhishek Potnis [:avp]]

Hi Yoric,

I would like to take up this bug again ! Will attach a patch soon !

Thanks.

Comment 51

[David Teller [:Yoric] - still alive but not very active]

I believe that this is too much effort spent on something that doesn't deserve it.