Last Comment Bug 438599 - the currentEngine attribute of nsIBrowserSearchService should never be null
: the currentEngine attribute of nsIBrowserSearchService should never be null
Status: RESOLVED FIXED
[hijacking][fxsearch]
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All All
-- major with 1 vote (vote)
: Firefox 43
Assigned To: Florian Quèze [:florian] [:flo] (PTO until February 27)
:
: Florian Quèze [:florian] [:flo] (PTO until February 27)
Mentors:
: 1088646 1232349 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-11 06:53 PDT by Timwi
Modified: 2016-04-08 03:37 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch (3.14 KB, patch)
2015-09-03 07:36 PDT, Florian Quèze [:florian] [:flo] (PTO until February 27)
dtownsend: review+
Details | Diff | Splinter Review

Description User image Timwi 2008-06-11 06:53:31 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0

In Firefox 3.0 RC 2, the "View Selection Source" item in the right-click menu on a webpage (with some text selected) doesn't do anything. No selection source, not even an error message.

Reproducible: Always
Comment 1 User image Ria Klaassen (not reading all bugmail) 2008-06-12 01:30:32 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008061105 Minefield/3.0pre

This works fine for me. Have you tried:

- Firefox's safe-mode to exclude extension/theme problems
- a new profile
- a reinstall in a new empty folder? 

http://kb.mozillazine.org/Safe_Mode_(Firefox)
http://kb.mozillazine.org/Profile_Folder
Comment 2 User image Timwi 2008-06-12 08:04:40 PDT
Apparently continually upgrading from the Firefox 3 betas to the Release Candidate managed to leave the "searchplugins" sub-folder in the Firefox installation path empty. With any single file in that sub-folder, things work as expected. With the folder empty, two behaviours change:

* right-click menu is sometimes ten times larger (contains every possible item from every possible right-click menu, including "View Image" even though I didn't right-click an image), sometimes the standard webpage right-click menu (which doesn't include "View Selection Source" or anything else context-specific). Couldn't find a pattern between the two.

* "View Selection Source", if present, is dead.
Comment 3 User image Jesse Ruderman 2008-06-14 01:18:35 PDT
Any error messages in the Error Console when this happens?  Especially ones from browser.js or other chrome JS?
Comment 4 User image Wayne Mery (:wsmwk, NI for questions) 2010-06-11 16:51:06 PDT
please retest with newer version.  Close, or update comment/attach testcase as appropriate to test results. Thanks
Comment 5 User image Timwi 2010-06-12 08:30:39 PDT
Still happening exactly as described.
Comment 6 User image Roman 2011-09-12 07:17:39 PDT
9.0a1 (2011-09-11): still happens.

STR:
- Get nightly with a clean profile
- Delete everything from "searchplugins"
- Run the nightly
- Go to www.google.co.uk/search?q=asdf
- Select some text
- Right-click.

Expected: a proper menu with Select all, Copy, Search (greyed out) and View Selection Source.

Actual: Sometiems a menu about two screens tall with every single menu item there is. Other times a small menu with irrelevant items.
Comment 7 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-06-11 04:02:27 PDT
The problem here is that the search service's default engine is null.

Removing the files directly from the searchplugins folder won't be possible anymore now that bug 1162569 is fixed.

Another way to reproduce is to remove all the search engines from the preferences UI, except the last one that the UI won't let you remove, and then from the browser console execute:
Services.search.removeEngine(Services.search.defaultEngine)

Then the context menu issue is that http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js?rev=e7875576b042#1742 breaks when ss.defaultEngine is null.

I'm not sure we should fix it in the context menu code though, it could be better to ensure the search service never reaches the state of not having a default engine.
Comment 8 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-09-03 07:27:09 PDT
Morphing the bug a little bit so that it's about the solution that we need, rather than a specific instance of the problem.

A faster way to reproduce is to execute this code:
Services.search.getVisibleEngines().forEach(Services.search.removeEngine);
Comment 9 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-09-03 07:36:41 PDT
Created attachment 8656591 [details] [diff] [review]
Patch

The original default engine should never be null (now that they are part of the omni.ja file, they are unlikely to ever be physically removed from the disk), so we don't need to have a built-in fallback engine, we can just unhide the original default, and the fix turns out to be very simple.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea8988dcf4e4
Comment 10 User image Dave Townsend [:mossop] 2015-09-03 09:26:25 PDT
Comment on attachment 8656591 [details] [diff] [review]
Patch

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

::: toolkit/components/search/tests/xpcshell/test_currentEngine_fallback.js
@@ +20,5 @@
> +  // Verify the original default engine is used as a fallback and no
> +  // longer hidden.
> +  do_check_eq(Services.search.currentEngine.name, currentEngine.name);
> +  do_check_false(currentEngine.hidden);
> +  do_check_eq(Services.search.getVisibleEngines().length, 1);

This is a little weird API-wise, would it be better to just have removeEngine throw on the attempt to remove the last visible engine?
Comment 11 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-09-03 09:40:16 PDT
(In reply to Dave Townsend [:mossop] from comment #10)

> This is a little weird API-wise, would it be better to just have
> removeEngine throw on the attempt to remove the last visible engine?

Not really. Downsides of that approach would be:
- wouldn't fix existing broken profiles. (Telemetry shows we have a non-trivial number of users who have 'NONE' as the reported default engine)
- would leave users with the last engine of the list set as the default, which is probably not an engine people would want to have (one may end up with twitter or ebay becoming the default for no good reason).
Comment 13 User image Wes Kocher (:KWierso) 2015-09-04 15:15:02 PDT
https://hg.mozilla.org/mozilla-central/rev/3a6613c338c2
Comment 14 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2015-09-16 08:17:08 PDT
*** Bug 1088646 has been marked as a duplicate of this bug. ***
Comment 15 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2016-04-08 03:37:39 PDT
*** Bug 1232349 has been marked as a duplicate of this bug. ***

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