438599 - the currentEngine attribute of nsIBrowserSearchService should never be null

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Timwi]

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

[Ria Klaassen (not reading all bugmail)]

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

[Timwi]

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

[Jesse Ruderman]

Any error messages in the Error Console when this happens?  Especially ones from browser.js or other chrome JS?

Comment 4

[Wayne Mery (:wsmwk)]

please retest with newer version.  Close, or update comment/attach testcase as appropriate to test results. Thanks

Comment 5

[Timwi]

Still happening exactly as described.

Comment 6

[Roman]

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

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

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

[Florian Quèze [:florian]]

Attachment: 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

[Dave Townsend [:mossop]]

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

[Florian Quèze [:florian]]

(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 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3a6613c338c2

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a6613c338c2