Closed Bug 1044577 Opened 5 years ago Closed 5 years ago

Create proof-of-concept hotfix add-on for overriding the list of known alternate domains for search engines

Categories

(Firefox :: Search, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: Paolo, Assigned: alexbardas)

References

Details

Attachments

(7 files, 8 obsolete files)

20.25 KB, patch
Paolo
: review+
Paolo
: checkin-
Details | Diff | Splinter Review
190.06 KB, image/png
Details
92.83 KB, image/png
Details
81.37 KB, image/png
Details
138.50 KB, image/png
Details
14.93 KB, application/x-xpinstall
Details
1.44 KB, patch
alexbardas
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
The list of known alternate domains for search engines, defined in bug 1040721, should be easily overridable using a hotfix add-on that overrides the SearchStaticData module.

This bug is about creating a proof-of-concept add-on that demonstrates this.
Flags: firefox-backlog+
Points: --- → 5
QA Whiteboard: [qa-]
I can start working on it. Can you please provide more info about which firefox versions need to be supported by it or other details?
Bug 1040721 landed in 34, which you can tell by looking at the Target Milestone field in that bug, so the proof of concept add-on should apply to versions 34 and greater.
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Attachment #8473359 - Flags: review?(paolo.mozmail)
I've tried different approaches and ask on #extdev and found out that in order to change / override a resource I have to do it at runtime.

Another approach I see would be to override the getAlternateDomains method in the hotfix addon, but I think it would be harder to test & maintain.

Paolo, what are your thoughts about what's best to be done here? (if this approach is ok, I'll add some tests for modifyDomainSource).
Attachment #8473362 - Flags: review?(paolo.mozmail)
Comment on attachment 8473359 [details] [diff] [review]
Override list of known alternate domains for search engines

(In reply to Alex Bardas :alexbardas from comment #4)
> Another approach I see would be to override the getAlternateDomains method
> in the hotfix addon, but I think it would be harder to test & maintain.

The general idea is that this function can apply pretty much any logic based on the domain name, so the add-on should modify the function's code rather than the data it works on.

For the moment we only implemented the Google logic, but for example we could add more engines with different rules.

I think that adding bootstrap code to override the "resource" URL, even dynamically, would work better since in most cases we'll just be able to copy the entire module from an already tested Nightly version and put it in the add-on skeleton, and the hotfix is ready.
Attachment #8473359 - Flags: review?(paolo.mozmail)
Attachment #8473362 - Flags: review?(paolo.mozmail)
I hope I got it right this time. I've copied the SearchStaticData module inside the hotfix add-on and changed `gGoogleDomainsSource` (as a proof of concept).

In order to override the SearchStaticData resource at runtime, we cannot do something like SearchStaticData = SearchStaticDataHotfix. The reason is here: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Using#Sharing_objects_using_code_modules 

"Note: Each scope that imports a module receives a by-value copy of the exported symbols in that module. Changes to the symbol's value will not propagate to other scopes (though an object's properties will be manipulated by reference)."

So I've just overwritten the initial properties of SearchStaticData.
Attachment #8473359 - Attachment is obsolete: true
Attachment #8473362 - Attachment is obsolete: true
Attachment #8474334 - Flags: review?(paolo.mozmail)
Iteration: --- → 34.2
Comment on attachment 8474334 [details] [diff] [review]
Hotfix addon which overwrites all properties from SearchStaticData

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

This looks quite good, thanks!

::: v20140814.01/bootstrap.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

nit: the correct license header has "file" on the next line.

@@ +11,5 @@
> +function overwriteResourceHandler(data) {
> +  const ioService = Cc['@mozilla.org/network/io-service;1']
> +                      .getService(Ci.nsIIOService);
> +  const resourceHandler = ioService.getProtocolHandler("resource")
> +                                   .QueryInterface(Ci.nsIResProtocolHandler);

You can just use "Services.io".

nit: we generally use "let" even if the result won't change, like you do below.

@@ +17,5 @@
> +  let resourceURI = ioService.newURI(data.resourceURI.spec + "/resource/", null, null);
> +  resourceHandler.setSubstitution("firefox-hotfix", resourceURI);
> +
> +  // And import the JSM as a side-effect.
> +  let module = Cu.import("resource://firefox-hotfix/SearchStaticData.jsm");

I think the word "side-effect" is not really clear, but the comment can just be removed, the code is self-documenting.

@@ +19,5 @@
> +
> +  // And import the JSM as a side-effect.
> +  let module = Cu.import("resource://firefox-hotfix/SearchStaticData.jsm");
> +
> +  if (module.SearchStaticDataHotfix) {

This check can be removed, the module will always have the object defined.

@@ +24,5 @@
> +    // Overwrite all SearchStaticData's properties with those redefined in the
> +    // hotfix module.
> +    for (let property in module.SearchStaticDataHotfix) {
> +      if (module.SearchStaticDataHotfix.hasOwnProperty(property) &&
> +          SearchStaticData.hasOwnProperty(property)) {

"for... of ...getOwnPropertyNames()", and no need to check whether the property exists in the destination module.

@@ +59,5 @@
> +}
> +
> +function shutdown(data, reason) { }
> +
> +function uninstall(data, reason) { }

We could unregister the resource substitution on shutdown / uninstall (not sure which one is better).

::: v20140814.01/chrome.manifest
@@ +1,3 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
\ No newline at end of file

Can the chrome.manifest file just be omitted?

::: v20140814.01/resource/SearchStaticData.jsm
@@ +22,5 @@
> +
> +// To update this list of known alternate domains, just cut-and-paste from
> +// https://www.google.com/supported_domains
> +const gGoogleDomainsSource = ".bing.com .yahoo.com";
> +const gGoogleDomains = gGoogleDomainsSource.split(" ").map(d => "www" + d);

To clarify the purpose, the function below is supposed to return all the variants related to the provided domain, but not unrelated domains. For example, if the function is passed any Bing domain, it will return a list of all Bing domains. The implementation currently has only one bucket, for Google, but future versions will work on several lists. How these lists are created may be different for each engine.

This is to say that a better example would include various domains from the same engine in the same list. It is not really important here, since this add-on is just a test, but this hotfix would have the side-effect of confusing Bing searches with Yahoo searches when installed for testing.

@@ +24,5 @@
> +// https://www.google.com/supported_domains
> +const gGoogleDomainsSource = ".bing.com .yahoo.com";
> +const gGoogleDomains = gGoogleDomainsSource.split(" ").map(d => "www" + d);
> +
> +this.SearchStaticDataHotfix = {

Keeping this named SearchStaticData is probably simpler.

I'm not familiar with how we test hotfixes, and in this case we don't need to deploy this until there is a need. How can we ensure this hotfix can be independently tested?

Maybe you can package a restartless add-on XPI with a different ID, and make it available for QA verification? (The test would be that Google locale redirects are not styled anymore as past search results in the location bar dropdown.)
Attachment #8474334 - Flags: review?(paolo.mozmail) → feedback+
I've added a method which copies all properties from one module / object into another module / object (I cannot just initialize the old module with {}, I have to delete all properties to have it working). 

With this method, I can set the old properties from SearchStaticData at uninstall.

I tried to rename StaticSearchDataHotfix >>> StaticSearcData, but the fix would not work anymore.

I think this is good for a new review, in the meantime I'll try to look into how this can be tested.
Attachment #8474334 - Attachment is obsolete: true
Attachment #8474803 - Flags: review?(paolo.mozmail)
Comment on attachment 8474803 [details] [diff] [review]
Hotfix addon which overwrites all properties from SearchStaticData

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

(In reply to Alex Bardas :alexbardas from comment #8)
> With this method, I can set the old properties from SearchStaticData at
> uninstall.

Good idea!

Note that the nsSearchService file might be caching the information, this means that the updated engine data might not be really available until a restart. We can verify this during testing.

::: v20140814.01/bootstrap.js
@@ +26,5 @@
> +                                   .QueryInterface(Ci.nsIResProtocolHandler);
> +  let resourceURI = Services.io.newURI(data.resourceURI.spec + "/resource/", null, null);
> +  resourceHandler.setSubstitution("firefox-hotfix", resourceURI);
> +
> +  let module = Cu.import("resource://firefox-hotfix/SearchStaticData.jsm");

> I tried to rename StaticSearchDataHotfix >>> StaticSearcData, but the fix
> would not work anymore.

Ah, you probably need to import the module into a dummy scope:

let module = Cu.import("resource://firefox-hotfix/SearchStaticData.jsm", {});

Otherwise, the SearchStaticData symbol in the global scope would be overwritten. I missed this in my previous review.

@@ +31,5 @@
> +
> +  cloneModule(gSearchStaticDataBackup, SearchStaticData);
> +  // Overwrite all SearchStaticData's properties with those redefined in the
> +  // hotfix module.
> +  cloneModule(SearchStaticData, module.SearchStaticDataHotfix);

I think this will be more robust:
- Always cycle on the properties in the "firefox-hotfix" module and:
  - Copy the originals to the backup object
  - Copy the hotfix properties to the original module

On uninstall:
- Always cycle on the properties in the "firefox-hotfix" module and:
  - Copy from the backup object to the original module

Since this is a simple loop, you can just do this inline, no need for a separate function.

In other words, any extra properties on the original module are unchanged.

@@ +63,5 @@
> +
> +function shutdown(data, reason) { }
> +
> +function uninstall(data, reason) {
> +  cloneModule(SearchStaticData, gSearchStaticDataBackup);

We'll need something like:

resourceHandler.setSubstitution("firefox-hotfix", null);

After the properties are restored.

I also forgot that we need a simple xpcshell test in-tree that ensures that the getAlternateDomains function can be overridden (with a comment explaining that this is needed for hotfixing). This will protect us from code accidentally freezing the object. This should be a separate patch, because we will need to land that in the source tree.
Attachment #8474803 - Flags: review?(paolo.mozmail)
Comment on attachment 8474803 [details] [diff] [review]
Hotfix addon which overwrites all properties from SearchStaticData

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

::: v20140814.01/install.rdf
@@ +10,5 @@
> +    <em:strictCompatibility>true</em:strictCompatibility>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>Firefox Hotfix</em:name>
> +    <em:description>Firefox hotfix to temporarily overrid the list of known alternate domains for search engines from SearchStaticData.</em:description>

overrid => override
I've implemented the suggestions and it seems to work pretty ok.
Attachment #8474803 - Attachment is obsolete: true
Attachment #8475302 - Flags: review?(paolo.mozmail)
Iteration: 34.2 → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Comment on attachment 8475302 [details] [diff] [review]
Hotfix addon which overwrites all properties from SearchStaticData patch #2

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

r+ with the two issues below addressed.

::: v20140814.01/bootstrap.js
@@ +56,5 @@
> +
> +function shutdown(data, reason) { }
> +
> +function uninstall(data, reason) {
> +  for (let property of Object.getOwnPropertyNames(gSearchStaticDataHotfix)) {

Ah, at this point we can iterate over gSearchStaticDataBackup and we don't require gSearchStaticDataHotfix anymore (sorry if I made it seem the algorithm I wrote should be taken literally, an equivalent solution works as well).

@@ +59,5 @@
> +function uninstall(data, reason) {
> +  for (let property of Object.getOwnPropertyNames(gSearchStaticDataHotfix)) {
> +    SearchStaticData[property] = gSearchStaticDataBackup[property];
> +  }
> +  resourceHandler.setSubstitution("firefox-hotfix", null);

I believe resourceHandler is undefined here. This reminds me that, as part of manual testing, we should ensure that we don't have errors in the console.

Can you please provide a list of installation/uninstallation steps for the hotfix, and a checklist for Quality Assurance to verify if everything works as expected?
Attachment #8475302 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8475557 [details] [diff] [review]
Xpcshell test to ensure that SearchStaticData methods can be overwritten

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

r+ with changes.

::: toolkit/components/search/tests/xpcshell/test_SearchStaticData.js
@@ +22,5 @@
> +  // needed for hotfixing.
> +  let backup = SearchStaticData.getAlternateDomains;
> +  SearchStaticData.getAlternateDomains = getAlternateDomains;
> +  do_check_true(SearchStaticData.getAlternateDomains("google.com")
> +                                .indexOf("bing.com") == 0);

I'd make some changes for clarity (sometimes people look at tests to figure out expected behavior):
- getAlternateDomains("www.bing.com")
- Return ["www.bing.fr"] and check it

nit: the external getAlternateDomains function can be inlined:

SearchStaticData.getAlternateDomains = () => ["www.bing.fr"];
Attachment #8475557 - Flags: review?(paolo.mozmail) → review+
Attachment #8475557 - Attachment is obsolete: true
Attachment #8476016 - Flags: review+
I'll come with some testing scenarios shortly (I've only tested with breakpoints and browser toolbox until now).
Attachment #8475302 - Attachment is obsolete: true
Attachment #8476101 - Flags: review+
I've moved the restoring logic from uninstall to shutdown. 

Because the domains get cached, restoring the SearchStaticData module is the same in both cases (a browser restart must be performed), but it certainly helps to just disable it from Add-on Manager while debugging.

From a user point of view, this change should have no effect.
Attachment #8476101 - Attachment is obsolete: true
Attachment #8476250 - Flags: review?(paolo.mozmail)
Steps to manually test the hotfix:

1. In order to change the default localization of the Search Engine (https://support.mozilla.org/en-US/questions/966542), visit http://mycroftproject.com/google-search-plugins.html.

2. Select Google DE - Das Web de-DE (google.de) by Mycroft Project (it doesn't actually matter which one of them is selected)

3. Make sure "Make this the current search engine is checked" (see [1]add_search_engine.png )

4. Add some search keywords in the awesome bar and press Enter (e.g. see [2]add_search_keywords.png)

5. Do an autocomplete and see how the results are displayed (see [3]hotfix_off_autocomplete.png)

6. Install the hotfix add-on (hotfix-v20140814.01.xpi) and restart the browser

7. Do the same autocomplete from step 5 and see how the results are displayed (see [4]hotfix_on_autocomplete.png). The results should be longer and uglier. If they look the same, something is not right.

The hotfix can be enabled / disabled from the Add-on Manager for more tests, but after each enable/disable, the browsers needs a restart.
Changed commit name and pushed to try for xpcshell tests:

https://tbpl.mozilla.org/?tree=Try&rev=a23b99064c9e
Attachment #8476016 - Attachment is obsolete: true
Attachment #8476368 - Flags: review+
Comment on attachment 8476250 [details] [diff] [review]
Hotfix addon which overwrites all properties from SearchStaticData patch #3

Thanks for the detailed test steps!

I add one step: verify that there are no errors in the Console after enabling/disabling the add-on.
Attachment #8476250 - Flags: review?(paolo.mozmail) → review+
I think this proof of concept reached its target. In the future & another bug, the dummy SearchStaticData module from hotfix should be obviously changed to be useful.
Keywords: checkin-needed
Comment on attachment 8476368 [details] [diff] [review]
Xpcshell test to ensure that SearchStaticData methods can be overwritten

Thanks! I've set the "checkin" flag to make it clear which of the two patches needs to be checked in.
Attachment #8476368 - Flags: checkin?
Attachment #8476250 - Flags: checkin-
Attachment #8476368 - Flags: checkin? → checkin+
https://hg.mozilla.org/integration/fx-team/rev/14a9a6cc93a3
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/14a9a6cc93a3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.