Closed
Bug 1044577
Opened 10 years ago
Closed 10 years ago
Create proof-of-concept hotfix add-on for overriding the list of known alternate domains for search engines
Categories
(Firefox :: Search, defect)
Firefox
Search
Tracking
()
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+
Updated•10 years ago
|
Points: --- → 5
QA Whiteboard: [qa-]
Assignee | ||
Comment 1•10 years ago
|
||
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?
Comment 2•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → abardas
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8473359 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 4•10 years ago
|
||
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)
Reporter | ||
Comment 5•10 years ago
|
||
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)
Reporter | ||
Updated•10 years ago
|
Attachment #8473362 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 6•10 years ago
|
||
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)
Updated•10 years ago
|
Iteration: --- → 34.2
Reporter | ||
Comment 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
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)
Reporter | ||
Comment 9•10 years ago
|
||
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)
Reporter | ||
Comment 10•10 years ago
|
||
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
Assignee | ||
Comment 11•10 years ago
|
||
I've implemented the suggestions and it seems to work pretty ok.
Attachment #8474803 -
Attachment is obsolete: true
Attachment #8475302 -
Flags: review?(paolo.mozmail)
Updated•10 years ago
|
Iteration: 34.2 → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8475557 -
Flags: review?(paolo.mozmail)
Reporter | ||
Comment 13•10 years ago
|
||
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+
Reporter | ||
Comment 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8475557 -
Attachment is obsolete: true
Attachment #8476016 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
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)
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
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+
Reporter | ||
Comment 25•10 years ago
|
||
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+
Assignee | ||
Comment 26•10 years ago
|
||
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
Reporter | ||
Comment 27•10 years ago
|
||
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?
Reporter | ||
Updated•10 years ago
|
Attachment #8476250 -
Flags: checkin-
Updated•10 years ago
|
Attachment #8476368 -
Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/14a9a6cc93a3
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•