299825 - Search plugins in App directory are updated into Profile directory (resulting in double Search Bar entries)

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Stebs]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050705 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050705 Firefox/1.0+

If an Search Plugin in the app dir's Searchplugin directory is automatically
updated, the Internet Search Service places the update into the Users Profile
directory, creating an .src file with the same Name as the one in the
application Searchplugin Directory and thus resulting in an double entry in the
Search Bar.

This happened to me (and others) with the Google Search Plugin.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Shayne Jewers]

Attachment: Screenshot

Heres a screenshot of the bug, the selected entry is the double entry

Comment 2

[Robert Strong (they/them - no direct email)]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050705 Firefox/1.0+ ID:2005070511

The google.src distributed with the trunk has an update available and the info
in comment #0 is accurate AFAICT.

Comment 3

[Asa Dotzler [:asa]]

This is gonna need to make b4 if it's gonna make 1.1 so shifting the nomination.

Comment 4

[Jaime Mitchell (use [e-mail] for email)]

*** Bug 301194 has been marked as a duplicate of this bug. ***

Comment 5

[O. Atsushi (Torisugari)]

Which is the better solution than others?

1. Plugins in appdir update themselves within appdir.
2. Plugins in appdir move themselves (and image files) to profile dir,
   when an update event fires.
3a. Plugins in appdir do not update.
3b. Move google plugin from appdir to the default profile, in the first place.

Comment 6

[Benjamin Smedberg]

Either 1 or 3a

Comment 7

[Aaron Kelley]

I think it makes sense for plugins in the app dir to update themselves in the
app dir, and plugins in the profile dir to update themselves in the profile dir.
   So...  1.

(Maybe I put a search plugin in the app dir so that all users on my PC would be
able to use it, it might be frustrating if it moves itself, but then...  It's
possible that you won't have permission to write to the app dir.)

Comment 8

[Andrew Ferguson]

(In reply to comment #7)
> I think it makes sense for plugins in the app dir to update themselves in the
> app dir, and plugins in the profile dir to update themselves in the profile dir.
>    So...  1.

If I delete some of the search plugins that come by default and then upgrade
Firefox, the plugins would re-appear under this scheme, no? Deleted plugins
should stay deleted ... which would mean moving all plugins out of the app dir
and into the (default) profile.

Comment 9

[Benjamin Smedberg]

> If I delete some of the search plugins that come by default and then upgrade
> Firefox, the plugins would re-appear under this scheme, no? Deleted plugins

Yes, and that is by design at this point. In the future you will be able to
disable the global searchplugins, but that's not the immediate goal.

Comment 10

[O. Atsushi (Torisugari)]

Attachment: Patch (Update files will be located in the same dir)

> 1. (Please take a look at comment #5)

Minimum patch, I think. This patch atatches original engine's RDFResource
(~= engine's file path) to the streamListener's context. And when
saveContents()
called, get the original file path from that context and save the new file into

the same directory.

The problem is it's not easy for me to test whether this patch really fixed
this bug. I guess original authors might have been with some fake HTTP header
responses... (Ping = HTTP "HEAD" request.)

3a would be a bit easier to write patch.

Comment 11

[Robert Strong (they/them - no direct email)]

I believe that you could test this by using a google.src from a build around the
time of the bug report. Since that time I believe the google.src in the tree now
points to mozilla.org and not to google which is where the updated google.src
was located. I suspect there are other ways but I haven't looked at how the code
determines an update is available.

Comment 12

[Aaron Kelley]

Attachment: old google.src that will trigger an update

In response to Comment #11...  Here is the google.src from Deer Park Alpha 2
which points to Google for an update (so you don't have to go dig one out if
you want to test this).

Comment 13

[Stebs]

Just had the same idea and tested with an google.src from an old nightly
2005060506, its google.src is the exact same as in the attachment. (verified
with WinMerge)
Could trigger the bug with that, but astonishingly it was the OLD version that
could be found in Profile?
Could not repeat it.(I guess because of "updateCheckDays=1")
Sorry for not providing an old google.src in the first place...

Comment 14

[O. Atsushi (Torisugari)]

Thank you very much. I've noticed this component is checking
not only last-modified but also content-length.

Steps to examine:
1. Edit google.src (e.g. replace with the attatchment 190795)
2. Restart browser and *use that plugin via search box*.
3. Wait 60 seconds.

Now it seems to me the patch is working fine.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 190675 [details] [diff] [review]
Patch (Update files will be located in the same dir)

style in this file is so fractured that I just don't care about tabs vs. spaces
in general, but it'd be nice to see tabs in functions with tabs, new stuff
should be spaces IMO.

>-  // Just like AddSearchEngine, but with an extra fifth parameter to say
>-  // whether it's our internal update or a new engine.
>+  // Just like AddSearchEngine, but with an extra fifth parameter.
>+  // If it's a new engine, the resouce should be null.

// Just like AddSearchEngine, but with an extra parameter to include the 
// nsIRDFResource of an existing engines.  If this is a new search engine
// aOldEngineResource should be null.

r=me fwiw, but let's get some bsmedberg/dbaron opinion on this.

IMO, this is the right approach, since we may have a need to update old
clients, and while we still can't get everyone, its the best we can do without
bigger changes to how we do search.

Comment 16

[O. Atsushi (Torisugari)]

Attachment: Patch (Fixed comments and tabs->spaces)

(In reply to comment #15)
> but it'd be nice to see tabs in functions
> with tabs, new stuff should be spaces IMO.

It drove me mad.
> /* -*- Mode: C++; tab-width: 2;...
is nonsense or just a lie, apparently. Then I'm
not too sure how many tabs I should put.

If you don't mind reviewing, I'd rather like to
replace all tabs in that file with spaces.
I've already spent some time for nothing...

Comment 17

[Mike Connor [:mconnor]]

Comment on attachment 190921 [details] [diff] [review]
Patch (Fixed comments and tabs->spaces)

This doesn't need another round of reviews, just approval.

Comment 18

[O. Atsushi (Torisugari)]

Attachment: Patch

Comment 19

[[e-mail]]

Comment on attachment 192164 [details] [diff] [review]
Patch

>+  PRUint32 engineFlag, iconFlag;
>+  if (aOldEngineResource) {
>+    // Update a plugin
>+    engineFlag = nsIInternetSearchContext::ENGINE_DOWNLOAD_UPDATE_CONTEXT;
>+    iconFlag = nsIInternetSearchContext::ICON_DOWNLOAD_UPDATE_CONTEXT;
>+  }
>+  else {
>+    // Add a new plugin
>+    engineFlag = nsIInternetSearchContext::ENGINE_DOWNLOAD_NEW_CONTEXT;
>+    iconFlag = nsIInternetSearchContext::ICON_DOWNLOAD_NEW_CONTEXT;
>+  }
I don't see the point of these, you only use the variables once; what was wrong
with changing isUpdate to aOldEngineResource in the ?: expression?

>+  rv = NS_NewInternetSearchContext(engineFlag, nsnull, aOldEngineResource,
>+                                   nsnull, suggestedCategory,
>+                                   getter_AddRefs(engineContext));
>+  NS_ENSURE_SUCCESS(rv, rv);
You sure like those NS_ENSURE_ macros, don't you?

>+  if (contextType == nsIInternetSearchContext::ENGINE_DOWNLOAD_UPDATE_CONTEXT ||
>+      contextType == nsIInternetSearchContext::ICON_DOWNLOAD_UPDATE_CONTEXT) {
Personally I would have tried context->GetEngine first, and assumed that if we
had an engine then we were updating it :-)

>+    nsCAutoString oldEnginePath;
>+    rv = EnginePathFromResource(oldResource, oldEnginePath);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsILocalFile> oldEngineFile;
>+    rv = NS_NewNativeLocalFile(oldEnginePath, PR_TRUE,
>+                               getter_AddRefs(oldEngineFile));
>+    NS_ENSURE_SUCCESS(rv, rv);
Might this be worth inlining into EnginePathFromResource?

>+InternetSearchDataSource::EnginePathFromResource(nsIRDFResource* aResource,
>+                                                 nsACString& aResult)
I'd like this to be an nsAFlatCString& at least. See below.

>+  nsCAutoString escapedPath, protocol;
>+  protocol.Assign(kEngineProtocol);
Don't copy kEngineProtocol into a new string. Please use an NS_LITERAL_CSTRING
or if that won't work an nsDependentCString instead.

>+  escapedPath.Assign(engineURI);
I'd assign this directly into aResult.

>+  nsUnescape(tempStr);
I'd use nsUnescapeURL(aResult); here - although the naming is really bad, I
know :-(

sr=me with these nits fixed.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

please use nsCString rather than nsAFlatCString

(also, it's NS_UnescapeURL, not nsUnescapeURL)

Comment 21

[O. Atsushi (Torisugari)]

Attachment: Patch

Thanks comments.

(In reply to comment #19)
> You sure like those NS_ENSURE_ macros, don't you?

Yes?

> you only use the variables once; what was wrong with changing isUpdate
> to aOldEngineResource in the ?: expression?

"There is an old RDF resource param" = "This func call is due to update"
is a bit tricky and it's hard to understand the code intuitively. That
was my concern.

Honestly speaking, I'd rather like to chage the interface.

1. Whether we are to download a engine or an icon and whether dwonload is
   update or not have no logical dependency. So we should use a
   combination of flags: DOWNLOAD_ENGINE/_ICON and DOWNLOAD_UPDATE/_NEW
   instead of a singl flag ENGINE_DOWNLOAD_UPDATE_CONTEXT

2. I'm afraid "get file path from rdf resource" isn't a good strategy.
   At most, we can't fix bug 284456, because rdf resource tells us
   only where the search engine is, but we do want to know the icon
   location as well. My patch assumes both engine and icon are in
   the same directory, but bug 284456 requires icon's exact file name,
   for, icon name on the web might be different from local leaf name.
   That is again and again tricky. Rather, nsIInternetSearchContext
   should *officially and explicitly* support where to save, with a
   new nsIFile property.

Comment 22

[[e-mail]]

Comment on attachment 192480 [details] [diff] [review]
Patch

(In reply to comment #21)
>Honestly speaking, I'd rather like to chage the interface.
Isn't hindsight a wonderful thing ;-)

Comment 23

[O. Atsushi (Torisugari)]

Checked in on both trunk and 1.8 branch.

Comment 24

[O. Atsushi (Torisugari)]

*** Bug 279025 has been marked as a duplicate of this bug. ***