109739 - plugins registered in appreg each time they are used (?)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[R.K.Aa.]

on linux i noticed that appreg in a profile created on Oct. 30th had grown to
2.5MB. Did a strings on it and counted how many times it mentioned various plugins:

grep QuickTime appreg.text |wc -l
   1668

grep x-java-applet appreg.text |wc -l
    790

This seems a little much? A new appreg suffices with mentioning these plugins
respectively 12 and 10 times, which is more sane as it reflects various so.files
and their associated file-extentions for mime types.

Comment 1

[R.K.Aa.]

the file "appreg.text" was one i created by running "strings appreg >appreg.text
(To look at what was there. The content of appreg itself is partly binary.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

reading that file can't be good for startup performance.  :)

Comment 3

[R.K.Aa.]

i don't think it affects startup actually..doesn't seem it's read then
but i think my plugins start quicker when the file is smaller - seems to be far
less disk activity since i regenerated it to a smaller version.

Comment 4

[R.K.Aa.]

wonder if this may have started accumulating after some old crash.

Been testing plugins all over the place now. Restarted several times: appreg
filesize remains sane. Hard to repro -> lowering severity.

This reminds me of the growing rfd files that could occure.

av: Old huge appreg available if you ever get the time to poke at it.

Comment 5

[Peter Lubczynski]

This extra size may be caused by caching plugin info found in the initial scan
at startup. Run regexport to dump the registry. In order to increase subsequent
startup time, all found plugins, valid or not, are cached in the registry. They
are only loaded at start time if their time stamp has changed from the registry.

Comment 6

[R.K.Aa.]

I install nightlies regularly. The plugin dir is wiped clean then, so i copy
over plugins again. I assume this means the appreg increments on each install,
unless i created a new profile each time too. Which I don't - I'm pretty pleased
with the one i have.. So this is the expected behaviour then? If so, please
resolve as invalid.

Comment 7

[av (gone)]

--- Mass reassigning Unix bugs to serge ---

Comment 8

[serge (gone)]

I think this is all platforms bug, at least my w2k registry.dat is getting fat 
too, which probably decrease a performance:(
I'm not sure when and how we are doing registry packing, I think it should be at 
installation time.
--> dveditz.

Comment 9

[Daniel Veditz [:dveditz]]

Back to you... you guys are mis-using the registry format. The interface
promises a lot more than the implementation delivers since we only filled it out
as far as was needed. In particular "RemoveSubtree()" is a huge red flag since
that strands records in the file marked deleted but never recovered.

The registry is slow and inefficient. I strongly urge you to consider some other
data format, like a text file. If you insist on using it then you'll have to
implement the registry packing API and expose it to nsIRegistry. It'll still be
slow, but at least it won't grow.

Comment 10

[serge (gone)]

>The registry is slow and inefficient. I strongly urge you to consider some 
>other data format, like a text file.
dp, what do you think?

Comment 11

[Suresh Duddi (gone)]

Current code reads and writes to the persistent store (appregistry now) in full
and not in parts. So this would work nicely. But I am not sure about the
priority of this or whether we should do that.

If packing is all we need to do at specific instances, then exposing the pack
api and doing it is simple. The entire reason this application registry exists
to for application to note down persistent data - infrequently changing data.
Plugin information does not change frequently. So this is the right data
storage. It is conceivable there will be more customers of the appregistry.

So I think doing the pack is a better way to go. Or we should rewrite
appregistry's implementation.

Disadvantages of another file:
- Another file open/read/write. Bad for mac.

Comment 12

[Peter Lubczynski]

I don't know much about it, but what about using RDF to store plugin information
and extending mimetypes.rdf for plugin handlers? Does RDF have similar
performance problems as the app registry? I'd be nice to tie in helper
application preferences with plugins.

Comment 13

[Daniel Veditz [:dveditz]]

Using the registry isn't so bad if you change the data structure you're using so
you don't delete and re-write every start. For example, instead of keying off
generic labels like "plugin-1" which change their meaning, key off a filename or
MIME type.

If you need to you can delete things which are gone for good, just don't delete
"just in case" things have gone away.

Comment 14

[Suresh Duddi (gone)]

My understanding of frequency of write in user senario:
- On install
- whenever plugins are added/removed

If we are writing to the registry more often, that is a bug.

Comment 15

[Peter Lubczynski]

--->dougt says he wants this

Comment 16

[Doug Turner (:dougt)]

Attachment: Key plugin by its given name.

This patch removes the anonymous plugin key and usings a key based on the
plugin's name.	I could not think of an instance where an application of two
plugins of the same name registered in two different locations.  If this
support is required, which I doubt, we could used the plugin's location on disk
as the key.  In either way, we will have a reliable way to verify that the key
in the registry is indeed the plugin we are looking at.

An overview of the patch:

AddPluginInfoToRegistry no longer needs to have the |keyname| passed in as it
will use the |tag->mName|.

Removal of the RemoveSubtree().  If we fail, there may be valid cached entries
in the cache.  We should be more optimistic.

Since we don't need to toss the subtree anymore, I removed
|registry->RemoveSubtree| from CachePluginsInfo().

From the same function, I removed the code which generated the anonymous key.

So, with these changes, I think that we should not be growing the registry
anymore.  Dan, am I missing someting?
Plugin folks, please comment on paragraph (1).

Comment 17

[av (gone)]

Doug, there could be duplicate plugins in different locations and all must be 
present in plugin info cache, we cache info on all plugins including 'unwanted' 
and duplicate.

Comment 18

[Doug Turner (:dougt)]

okay.  so, we can use the key:

  rv = registry->AddSubtree(top, tag->mFileName, &pluginKey);

In this way, every plugin will be reflected.  The only draw back is that you
will have this as the subtree key AND in utf8 node data.  But that is not nearly
as bad bloat without this patch.  :-).  

Av, with this change, can I get your r=?

Comment 19

[av (gone)]

Comment on attachment 69581 [details] [diff] [review]
Key plugin by its given name.

r=av given dup plugins are reflected.

Comment 20

[Daniel Veditz [:dveditz]]

You won't be able to AddSubtree() with a filename, because the filename may have
slashes or even illegal chars.

There is an AddSubtreeRaw, though, if you need it. THen you'll have to change
your enumeration to one that can handle "raw" unprocessed nodes.

Comment 21

[Suresh Duddi (gone)]

-  // We dont want any old plugins that dont exist anymore hanging around
-  // So remove and re-add the root of the plugins info
-  nsresult rv = registry->RemoveSubtree(nsIRegistry::Common, kPluginsRootKey);

I dont understand how this is going to work. Say the user removed a bunch of
plugins, then this will cause the removed plugins to still exist in our registry.

The reason we did "plugin-0" for the keyname was 'cause of restriction on what
the key can have. (See Dan's comment and Dan stop causing collisions ;-)

Overall, I am confused at what problem this patch is solving.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 69581 [details] [diff] [review]
Key plugin by its given name.

>+  // key by the plugin name.
>+  rv = registry->AddSubtree(top, tag->mName, &pluginKey);

This will surely break. For one thing the narrow char apis are UTF8,
and will fail if given invalid UTF8 data. Second, if you change it
to use the full path name because of the duplicates problem then any
slashes in the name will create sub-nodes which probably isn't what
you want.  The AddSubtreeRaw() will take care of the slash problem,
but not the UTF8 problem.

bug 28298 is a placeholder for finishing up the work on nsIRegistry.
the current AddSubtree() -> AddKeyUTF8(), and we need to add a Unicode
AddKey(). (extend to all the "subtree" APIs).


>   registry->SetStringUTF8(pluginKey, kPluginsFilenameKey, tag->mFileName);
>   if (tag->mFullPath)
>     registry->SetStringUTF8(pluginKey, kPluginsFullpathKey, tag->mFullPath);
> 
>   // we use SetBytes instead of SetString to address special character issue, see Mozilla bug 108246
>   if(tag->mName)
>     registry->SetBytesUTF8(pluginKey, kPluginsNameKey, nsCRT::strlen(tag->mName) + 1, (PRUint8 *)tag->mName);

Not your code, but can't give sr= with this glaring problem. The comment
shows someone knew about the UTF8 problem at one point (there's even a bug
reference), but there are two broken places right above that. Those
SetStringUTF8
should be SetBytesUTF8, or convert the name and path to Unicode and use the
Unicode SetString (which will convert to UTF8 internally, so double conversion
cost).


>   // Add in each mimetype this plugin supports
>   for (int i=0; i<tag->mVariants; i++) {
>     char mimetypeKeyName[16];
>     nsRegistryKey mimetypeKey;
>     PR_snprintf(mimetypeKeyName, sizeof (mimetypeKeyName), "mimetype-%d", i);

Why not key mimetypes by their mimetype similar to the above? might make
it easier to discard old unsupported ones.

>-  // We dont want any old plugins that dont exist anymore hanging around
>-  // So remove and re-add the root of the plugins info
>-  nsresult rv = registry->RemoveSubtree(nsIRegistry::Common, kPluginsRootKey);
>-  

You've solved the growth problem, but now we're back to the issue
that obviously concerned the original author here: how do you deal with
obsolete data in the cache?

Comment 23

[Daniel Veditz [:dveditz]]

The problem this patch is solving is that people are ending up with Megabyte
sized  (and growing) profile registries, with only a few K actual data in them.

There are alternate solutions:
 - implement the registry packing feature, and run it often
   (registry is locked while this happens, so make sure you're the only consumer)
 - Use a flat file for this data instead, or RDF, or mork, or whatever.

Comment 24

[Suresh Duddi (gone)]

Ok I need to rephrase my question. It does solve the problem but removing the
RemoveSubtree() is going to introduce another problem - every plugin ever
registered is going to stay in our registry, get read on startup and acted upon
when a data of that mimetype shows up. And hence the call being there in the
first place.

Comment 25

[Suresh Duddi (gone)]

Attachment: Using Pack()

Turns out that we already are exporting pack via nsIRegistry interface. Here is
a simple 4 liner. I haven't tested it. Dan, do you think this would work.

Comment 26

[Daniel Veditz [:dveditz]]

We're exporting it, but it's not actually implemented :-( (see bug 10210)

Implementing it shouldn't be too hard. The old 4.x code is still there ifdef'd
out, but it's probably easier to write it correctly from scratch. One problem we
had in 4.x was how slow it was.. if you set buffer size to the size of the file
in question (on both the old and new registries) that problem may be reduced.

Basically you just enumerate through the registry, and for each key enumerate
its entries, writing that into a new registry

The patch looks like it'd solve this growth problem without reintroducing the
"old plugin" problem (assuming you implement Pack), but I'd start to worry about
startup performance. When does this packing occur, and how often? My registry
grows all the time, so it appears we're invalidating and re-writing our cache
each run -- in that case is it really doing us any good?

Doug's approach of reusing predictable keys seems more efficient, assuming some
of the problems can be worked out. Obviously you'd have to enumerate the
existing data and compare to what you're going to write first to see if any
really should be deleted (applies to both plugins and mime-types within
plugins). Given the linked-list structure of plugin tags maybe that'd be a pain
to do (O(2) problems?).

Comment 27

[Suresh Duddi (gone)]

The plugin subtree will be written to only when plugins information change - new
plugins are added/deleted (or) someone calls javascript:plugins.refresh().

I am not beginning to like this pack thing. (why introduce new code)

I had another idea: How about writing all plugin information as a blob and
reading it as a blob and parsing it out. No subtree worries. 

Comment 28

[Doug Turner (:dougt)]

dan, can you post a patch which fixes up these call sites and does what you are
suggesting?

Comment 29

[Doug Turner (:dougt)]

dveditz - never mind.

Regarding obsolete data - I guess I could write a routine that stats() the
filename and if it is not there, remove that subtree?  We would still have
*some* bloat in this file doing this.  Thoughts?

Assuming that is good enough, I need to fix up the unicode key handling.  What
else is missing?

Comment 30

[Suresh Duddi (gone)]

Dougt, if you are going that route how about storing a attr/value like:

currentToggleStamp : 1

both in the top and in each plugin when you write entried to the subtree. The
next time you write plugins to registry, do something like:

- get("currentToggleStamp", &toggleStamp);
- toggleStamp = (toggleStamp ? 0 : 1);
- write plugins to registry with this toggleStamp and update top toggleStamp.
- Do another pass and remove any plugin subtree that has a mismatched togglestamp

This should be cheaper than stats. Esp if you plan on stating everything again
(not just the removed plugins).

Comment 31

[Doug Turner (:dougt)]

Dan, if I convert the two callers of SetStringUTF8 to SetBytesUTF8, will that
break existing clients?

Comment 32

[Daniel Veditz [:dveditz]]

Which patch are we talking about? If old clients try to GetString on bytes data
they'll get errors, yes, but I'm sure the wholesale changing of keys will be a
bigger problem for them.

Just change the root key you use and then old and new clients will live in
entirely separate namespaces. The new client will have to do a one-time read of
all the plugins, but since this bug was about how the file grew all the time it
sounds like we scan for plugins a lot anyway.

Comment 33

[Doug Turner (:dougt)]

passing to neeti.

Comment 34

[basic]

shouldn't this be also done for the mozilla0.9.9 branch?

Comment 35

[Doug Turner (:dougt)]

Neeti, what is the status of this bug?  

Comment 36

[Gagan]

[adt needinfo] what's the net effect of this? How risky is the fix? What is the
fix... what is the meaning of life? :)

Comment 37

[neeti]

One way to solve this is to not use the registry at for plugins, and use a 
flatfile structure and rewriting the caching code. I think this is risky.

The second option is to incorporate dveditz, and dp's suggestions above in the 
above patch. In order to make the plugins filename as the key, we would need to 
use AddSubtreeRaw(...) instead of AddSubtree(...). THen we will have to change
the enumeration to one that can handle "raw" unprocessed nodes Per dveditz, this 
would solve the will take care of the slash problem, but not the UTF8 problem.

dveditz,dougt: I need clarification on what exactly needs to be done to solve 
the UTF8 problem? How much work does this involve?

Then we also need to convert the two callers of SetStringUTF8 to SetBytesUTF8. 
To not break existing clients, we will have to change the root key.

Regarding obsolete date, need to incorporate dp's suggestion about 
currentToggleStamp.

Comment 38

[[not reading bugmail]]

regarding comment #16, #17, we now have the ability to put plugin files in both
mozilla.org/mozilla/plugins and the user's application/data/mozilla/plugins
directories.. the first is what is normal case, the second is for installing
multiple builds without having to move and copy plugins to a new build.

-dennis

Comment 39

[[not reading bugmail]]

Also note:

I just moved my plugins out of my current build directory to my application
directory on build 2002-3-13, and it causes my hard drive to thrash less than
having them in the build directory/plugins.. the only plugin needed in that
directory is the one that comes with mozilla, the downloader plugin.  Once I
took it out of the build directory, mozilla couldn't find my plugins, and I got
a dialog to come up telling me about the downloader plugin is needed when I was
on a page that had shockwave or wmp content.  So we know that works.. so
anyway.. just hopes this helps you understand and fix the problem that much more.

Comment 40

[Winnie Lam]

plusing this bug as per adt plugins triage.

Comment 41

[Dave Barrowman]

topembed+ per EDT triage.

Comment 42

[R.K.Aa.]

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

Comment 43

[Colin Blake]

about:plugins is taking 20 seconds on OpenVMS, I think because of appreg bloat. 
See bug 130322 for more details. Feel free to mark 130322 as a duplicate or a 
"depends on" if you think appropriate.

Comment 44

[Olivier Cahagne]

sorry for the spam, but I think my report is worth it: I've been using nightly
builds every day for 1 year, and my registry.dat is 20.8MB large (!). I didn't
touch it, perhaps I can be useful by measuring some loading time bench ?

Comment 45

[neeti]

Attachment: patch1

Comment 46

[neeti]

Using a flat file for storing plugin info instead of using libreg -

This patch uses a flatfile for storing plugin information. This is currently 
being stored in registry.txt/appreg.txt.

Instead of reading and writing the plugin information to registry.txt, we read 
and write from a flatfile pluginreg.dat which has the following format.


[HEADER]
version: x.x.x

[PLUGINS]

filename
lastModTimeStamp,description,name
mimetype1,description1,extension1
mimetype2,description21,extension2


TODO's

-Need to test these on linux and mac

Comment 47

[Gagan]

Comment on attachment 80814 [details] [diff] [review]
patch1

- how about using \, or %, for escaping the comma. 
- indentations seem messed up. are you using evil tabs? 
- goto's can be avoided. just a nitpick though.

Comment 48

[neeti]

Attachment: revised patch

Attached a revised patch for review.

Comment 49

[Peter Lubczynski]

Comment on attachment 81738 [details] [diff] [review]
revised patch

Looks pretty good. 

Question: What's going to happen to the existing plugin info people's appreg's?

Can we avoid the goto's?

Can you update to the trunk tip so I can try out on my Mac? I'm wondering what
happens with mFilename/mFullpath?

Comment 50

[neeti]

Attachment: revised patch updated with the latest trunk tip

Attached patch updated with the latest trunk tip. Removed the goto's. With my
current patch the existing plugin info in people's appreg's does not change. We
will not be reading or writing the plugin info to the appreg file.

Comment 51

[Cyrus Patel]

Is there a real need to cache the list of plugins? Is it so much faster to 
cache the filenames/dates/mimetypes than to determine this information at run-
time (the first time any plugin is required)? Is the trade-off (flexibility)
worth it? 

Even if all of the above are true, the list of plugins doesn't belong in the 
registry. The plugin record is specific to a particular installation (read: 
machine), while the registry is per-user. 

I suggest putting the record in the same directory as the plugins themselves.
  - The list of plugins would be specific for that set of plugins. Things 
    will be fine even when the user's home directory is not on the same 
    machine as the Moz installation (think AFS and/or NT roaming profiles).
  - There would be no need to have canonical paths pointing to the plugins.
    Things will be fine even if the Moz installation "moves" (or the mount
    point is different).
  - Shared installations of Moz will not require users to do an
    about:plugins to update their registries.
  - The file can be in "native" format. Since plugins are not binary 
    compatible accross platforms, there is no reason why the record 
    needs to be XP safe either.
  - requiring superuser privileges to update the list of available plugins 
    is a good thing. Since it takes admin privileges to install (shared)
    plugins, requiring privs to update the record is ok.
    (incidentally, Moz could optimize the frequency with which it scans 
    for new plugins by first considering if the record is writable.)

Comment 52

[WD]

>The plugin record is specific to a particular installation (read: 
>machine), while the registry is per-user. 

I do believe that mozilla supports per-user plugins, FWIW.   I have the
crossover plugins installed in my ~/.mozilla/plugins
Since it's a single-user only plugin (issues with WINE), it makes no sense for
me to have it installed system-wide.   (Otherwise mozilla crashes for other users)

Comment 53

[Peter Lubczynski]

Comment on attachment 82536 [details] [diff] [review]
revised patch updated with the latest trunk tip

r=peterl with the note that there might be old plugin cruft left in the appreg
from older browsers.

Caching this information is important for performance because some platforms
need much overhead to use NP_GetMIMEDescription() for getting MIME types.

Comment 54

[Chris Waterson]

*Sigh*! So, we're writing a bunch of new code because we don't want to finish
writing some code somewhere else? We're opening yet-another-file on startup? I
think this approach is wrong, wrong, wrong, and would rather see someone finish
implementing the registry's Pack() function.

I don't think this should be EDT+, ADT+, topembed+, or anything+. It only
manifests itself when someone regularly re-installs version after version after
version on a daily basis. Not a common user scenario, and something that a
Mozilla developer can deal with by clobbering the file (which, happily, will
rebuild itself). Removing said keywords for re-evaluation.

Comment 55

[Doug Turner (:dougt)]

Although it is not apparent in the bug, i removed most of the requirement for
libreg in another bug (48888).  I asked neeti to rip out the plugin/libreg
requirement in hopes to remove libreg completely at some point.

Comment 56

[R.K.Aa.]

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

Comment 57

[Daniel Veditz [:dveditz]]

Re comment 54

Unfortunately, Chris, this plugin information is written to the same cryptic
binary file that the profile data is (which should arguably be in a
human-readable format). If you delete the file to reset the plugin cruft you
also delete knowledge of your profiles.

Crucial data like profile locations should never have been in a non-editable
file, but that mistake was compounded when other buggy code started putting
their data in the same file.

Comment 58

[R.K.Aa.]

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

Comment 59

[R.K.Aa.]

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

Comment 60

[av (gone)]

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

Comment 61

[Matthias Versen [:Matti]]

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

Comment 62

[Peter Lubczynski]

This problem has become worse, it now seems to happen on every Netscape launch.
See bug 157627 and http://bugscape.netscape.com/show_bug.cgi?id=17790

Could we use 4.x #ifdef'ed Pack() function to "clean" the registry? There is
also a patch in bug 128366 to not load the plugin module at startup.

Comment 63

[Andrew Hagen]

Like you're saying, this seems to block bug 157627. Normal severity. Seems like
a high priority bug. 

Comment 64

[Jaime Rodriguez, Jr.]

renominating for topembed and making ADT1, as this is a serious problem, that
may corrupt users
profile.

this bug maybe related to bug 128366, and bugscape bug
http://bugzilla.mozilla.org/show_bug.cgi?id=128366

Comment 65

[Doug Turner (:dougt)]

neeti is out.  I will take.  You just want this fixed on the TRUNK right?

Comment 66

[rubydoo123]

as requested by ADT, I am adding a note to reference other related bugs:
bugscape bug 17790, assigned to AV: registry.dat grows on every start-up

bugzilla bug 157627, assigned to AV: bugzilla version of bugscape bug 17790

bugzilla bug 128366, assigned to peterl: OJI causes plugin DLL to load at
startup, scan for plugins, and load Java  DLL's

Comment 67

[rubydoo123]

Doug: I would suspect that we need this fixed on the trunk and the branch

Comment 68

[Doug Turner (:dougt)]

Attachment: update of neeti's work

Comment 69

[Doug Turner (:dougt)]

Attachment: xpcom part of the patch

Comment 70

[Doug Turner (:dougt)]

okay.

The plugin team is going to review this patch.  However, I do not recommend that
this patch land for the branch without much testing on the trunk.  This patch
basically rewrites how we store plugin information.

Comment 71

[serge (gone)]

Doug, last two patches are identical, and there is no xpcom part

Comment 72

[serge (gone)]

by xpcom part i mean changes in nsPluginHostImpl::LoadXPCOMPlugins()

Comment 73

[serge (gone)]

Attachment: slightly modified doug's patch

During review under debugger I've took a courage to modify dougt's patch a
little bit, to get rid of unnecessary Un/Escape calls I copied
nsManifestLineReader.h
into mozilla/modules/plugin/base/src/sPluginManifestLineReader.h and changed
',' delimiter to '|', and also I removed strdup/free from
nsPluginHostImpl::ReadPluginInfo. Please review.

Comment 74

[Jaime Rodriguez, Jr.]

doug/serge: we are trying to wrap up 1.0.1. assuming we are agreed that serge's
last patch is the best solution, can you work to get reviews, so that we could
get this landed asap. 

Comment 75

[Doug Turner (:dougt)]

Jamie read my comment #70.  Restated, I do not think that this patch should land
on the branch until there is *alot* more testing.  Given your schedule and the
above patch as the only proposed solution, I think that you may have to live
with this in the next release.  

Serge, your changes look good.  If you are happy with the inital changes, then I
am happy with yours.  Lets see about getting dveditz or alec to super review for
landing on the trunk.

Comment 76

[Daniel Veditz [:dveditz]]

Like Doug I'm concerned that this get thorough testing, but unlike Doug I don't
think we can pass on this for MachV. On the 7.0pr1 newsgroups people are
starting to notice 68Mb registry files. As mentioned before ee can't very well
tell users to delete the file because it also contains a pointer to their
profile, and they can't simply recreate *that* because of the random salt.

Serge's version of the patch does not include the escaping that Doug's does. We
can't trust that plugin info like description won't include format-busting
characters, and on Mac and Windows even the path could include ",". Doug's
escape function needs to escape the escaping character itself (and possibly \n
and \r in case of malicious/stupid plugin, although in that case we could argue
the user should delete the plugin instead because who knows how else it's
messing with their system).

Serge's patch doesn't do anything with "unwanted" plugins. Is there some reason
we don't need to cache those now?

Since you're currently rejecting the file if the version string doesn't match
exactly why not just compare the entire version line? It'd be tons cheaper than
parsing the line, converting strings to ints, and comparing each of the three
pieces individually.

There are a lot of places parsing the file where you appear to detect a bad
format but you try to carry on. Why not junk the file and load the plugins from
scratch? One example: if the mimetypecount is greater than your max (how?) you
reset it to max-1 and then read that many lines. If it were a correct number
you're now in an unexpected part of the file so you'll read junk, and if it was
a  bogus value then the file can't be trusted. Other places you simply break out
of reading the file early, but don't appear to note that fact in any way.

Ah, I see you changed the file delimiter to '|', probably to avoid having to
escape things. But that doesn't work: '|' might still show up in a description
somewhere. Any character you pick will have to be escaped because some fool will
find a way to use it. (might even be a common second byte value in a multibyte
asian character set, who knows?).

Comment 77

[av (gone)]

If we don't cache unwanted plugins we will re-write the cache info every time
even if nothing has changed. One way to avoid this is to check for unwantedness
not only when we read the info but also when we just scan to detect changes. But
the mechanism is quite fragile, I think the less we touch the better.

Comment 78

[R.K.Aa.]

bug 142247 is a fine sample of what can happen because moz wants to write to
appreg all the time.

Comment 79

[Jaime Rodriguez, Jr.]

dugt: Can we at least pursue reviews and get this landed on the trunk asap, just
in case?

Comment 80

[serge (gone)]

dveditz:
I used '|' as delimiter because on wi32 it is mimytype delimiter only stored in
plugin dll, on unix & mac the same delimiter is ';', I doubt the plugin vendors
use it in different way, 4.x & mozilla code wont be able to parse such plugins
info.I made that delim definable, and I think with an initial knowledge of the
input data we can bypass the escaping code and speed up file parsing a little bit. 

>Serge's patch doesn't do anything with "unwanted" plugins. Is there some reason
>we don't need to cache those now?
it does include tag->mFlags, which contains NS_PLUGIN_FLAG_UNWANTED bit, along
with some other bits we have to save.

>Since you're currently rejecting the file if the version string doesn't match
>exactly why not just compare the entire version line?
I haven't touch that code. Doug?

>if the mimetypecount is greater than your max (how?)
I use that max only for stack allocation of
+    char *mimetypes[PLUGIN_REGISTRY_MAX_MIMETYPES_PER_PLUGIN];
anyway I do not like that part of code it'll and I'm going to rewrite it.

>Other places you simply break out
>of reading the file early, but don't appear to note that fact in any way
that is true, as far as we break out before nsPluginTag* tag = new nsPluginTag()
call,we do not fill out our internal structure with bad data, and every thing is
going to be the same as in case new plugins were installed.

Comment 81

[Alec Flett]

so which attachment should I be reviewing? I see 3 non-obsolete patches here.

Comment 82

[serge (gone)]

Attachment: patch v2, addresses dveditz's concerns

Doug I've marked all prev patches as obsolete, to avoid confusions.
Alec, would you sr= on this patch, please

Comment 83

[Alec Flett]

Comment on attachment 92267 [details] [diff] [review]
 patch v2, addresses dveditz's concerns

sr=alecf but boy this is big for 1.0 - so there's NO chance of a simple fix
that will just prevent rewriting the registry every time?

anyway, will be good to see this land on the trunk anyway.

Comment 84

[Peter Lubczynski]

See bug 157627 for the "simple fix that will just prevent rewriting the registry
every time". There is patch there that is just a few lines and pretty safe but
the fix in this bug needs to be done sooner or later on the trunk anyway.
However, if the patch in bug 157627 really does do the trick for the meantime,
then perhaps this could wait till post 1.1beta as I think drivers approval is
needed on the trunk too these days.

Comment 85

[Daniel Veditz [:dveditz]]

Comment on attachment 92267 [details] [diff] [review]
 patch v2, addresses dveditz's concerns

>+  while (1)
>+  {
     ...
>+     if (!reader.NextLine())
>+       break;
>+   }

Instead of the icky while(1) which tells the reader nothing about the loop,
I'd prefer "do{...}while (reader.NextLine());", but I guess since this is
tested and reviewed code don't change it now. Fix the brace alignment though.

>+      //description\n  --\  on separate line, so there is no need to parse it
>+      //name\n         --/  and check for delimiters

Well, OK, but those weren't the only parts of the file that needed to
be escaped just in case there's some broken plugin out there. But I guess
we've got a version stamp in the file, so we can fix that later. It's
not a security hole because an evil plugin would do its work directly.
I'm mostly worried that someone will produce a plugin with descriptions
in an Asian multibyte charset, or install with problem chars in the path
(again, most likely with a multibyte charset OS).

File a bug on it ("plugin cache format needs escaping") and I'll let
it go. It's certainly a minor concern compared to the problem we're
trying to fix.

In reading the plugin cache, why not get the size and set
flen before opening it, so if those two actions fail you don't
have to do the extra closes. And then close the file immediately
after reading so everything doesn't have to jump to out:

Then you could have passed ownership of the buffer to the
manifest reader, and had it do the free in its destructor and
done away with all the goto outs. Much cleaner and less likely
to get broken during future maintenance. Again, since this is tested
code I'm ambivalent about asking for changes, so you could sway me
into accepting this as-is by filing a bug on the cleanup.

sr=dveditz if you file those two bugs. It'd be good to get
alec's sr= as well -- since testing time will be short the more
eyes on the code the better.

Comment 86

[av (gone)]

Comment on attachment 92267 [details] [diff] [review]
 patch v2, addresses dveditz's concerns

Looks like here we may write excessive info:

+  nsPluginTag *taglist[] = {mPlugins,mCachedPlugins};
+  for (int i=0; i<sizeof(taglist)/sizeof(nsPluginTag *); i++) {
+    for (nsPluginTag *tag = taglist[i]; tag; tag=tag->mNext) {

In this double loop we cache everything from |mPlugins| which contains 'wanted'
plugins and everything from |mCachedPlugins| which normally contains _only_
unwanted plugins. But it is possible that the latter list contains more info:
if some plugin had been deleted before we started the application, its info
will remain in |mCachedPlugins| list as it is not going to be removed from
there when we scan folders and find plugins, because this deleted plugin cannot
obviously be found. In current code this is addressed by checking the flag, so
something like the following should suffice:

+  nsPluginTag *taglist[] = {mPlugins,mCachedPlugins};
+  for (int i=0; i<sizeof(taglist)/sizeof(nsPluginTag *); i++) {
+    for (nsPluginTag *tag = taglist[i]; tag; tag=tag->mNext) {
+      // store each plugin info into the registry
+      //filename|fullpath|lastModifiedTimeStamp|canUnload|tag->mFlags
+      if ((taglist[i] == mCachedPluigns) && 
+	   !(tag->mFlags & NS_PLUGIN_FLAG_UNWANTED))
+	   continue;
     [...]

This will ensure that we count only for 'unwanted' plugins when caching the
list and don't cache the ghost plugin info.

Comment 87

[av (gone)]

Comment on attachment 92267 [details] [diff] [review]
 patch v2, addresses dveditz's concerns

One more nit. We will crash with empty plugin list. Please do something like:

+nsPluginHostImpl::WritePluginInfo()
 {
+ NS_ENSURE_ARG_POINTER(mPlugins);
+ NS_ENSURE_ARG_POINTER(mCachedPlugins);

I am marking this 'needs-work', please yell on me loudly if disagree.

Comment 88

[av (gone)]

Doug, once we decided on the approach to fix this, would not it make sense to
change the summary to something like 'Move plugin cache info from the
application registry to a flat file'? The problem which is described by current
summary is covered in bug 157627.

Comment 89

[Paul Wyskoczka]

Changing to adt2 per the adt.

Comment 90

[Brendan Eich [:brendan]]

Comment on attachment 92267 [details] [diff] [review]
 patch v2, addresses dveditz's concerns

>+  while (1)
>+  {
>+    if (*reader.LinePtr() == '[')
>+    {
>+       char* p = reader.LinePtr() + (reader.LineLength() - 1);

Indentation got off too far by one right here, three spaces instead of two over
the previous line.

>+       if (*p != ']')
>+         break;

Underindented, but correct if you fix the if and all the other lines in the
outer if's then-block.

>+       *p = 0;
>+
>+        char* values[1];
>+        if (1 != reader.ParseLine(values, 1))
>+          break;
>+        // ignore the leading '['
>+        if (0 != PL_strcmp(values[0]+1, token)){
>+          if (!reader.NextLine())
>+            break;
>+          else
>+            continue;
>+        }
>+        return PR_TRUE;
>+     }
>+     
>+     if (!reader.NextLine())
>+       break;
>+   }

dveditz, I see absolutely no risk (no known C or C++ compiler bug where
do{...}while(c) was not equivalent to while(1){...if(!(c))break;}.  This loop
should be a do-while loop, not a while (1) infinite loop (aside: canonical
infinite loops in C use for (;;){...}).

/be
>+   return PR_FALSE;
>+}
>+
>+////////////////////////////////////////////////////////////////////////
> // Little helper struct to asynchronously reframe any presentations (embedded)
> // or reload any documents (full-page), that contained plugins 
> // which were shutdown as a result of a plugins.refresh(1)
>@@ -1162,11 +1198,12 @@
>        (mVariants != aPluginTag->mVariants) )
>     return PR_FALSE;
> 
>-  if (0 != mVariants && mMimeTypeArray && aPluginTag->mMimeTypeArray)
>-    for (PRInt32 i = 0; i < mVariants; i++)
>+  if (0 != mVariants && mMimeTypeArray && aPluginTag->mMimeTypeArray) {
>+    for (PRInt32 i = 0; i < mVariants; i++) {
>       if (PL_strcmp(mMimeTypeArray[i], aPluginTag->mMimeTypeArray[i]) != 0)
>         return PR_FALSE;
>-
>+    }
>+  }
>   return PR_TRUE;
> }
> 
>@@ -5095,20 +5132,9 @@
> 
>   *aPluginsChanged = PR_FALSE;
>   nsresult rv;
>-  
>-  // If the create instance failed, then it automatically disables all
>-  // our caching functions and we will just end up loading the plugins
>-  // on startup.
>-  nsCOMPtr<nsIRegistry> registry = do_CreateInstance(kRegistryCID);
>-  if (registry) {
>-    rv = registry->OpenWellKnownRegistry(nsIRegistry::ApplicationRegistry);
>-    if (NS_FAILED(rv)) {
>-      registry = nsnull;
>-    }
>-  }
> 
>-  // Load cached plugins info
>-  LoadCachedPluginsInfo(registry);
>+  // Read cached plugins info
>+  ReadPluginInfo();
> 
>   nsCOMPtr<nsIComponentManager> compManager = do_GetService(kComponentManagerCID, &rv);
>   if (compManager) 
>@@ -5241,7 +5267,7 @@
>   // if we are creating the list, it is already done;
>   // update the plugins info cache if changes are detected
>   if (*aPluginsChanged)
>-    CachePluginsInfo(registry);
>+    WritePluginInfo();
> 
>   // No more need for cached plugins. Clear it up.
>   ClearCachedPluginInfoList();
>@@ -5457,55 +5483,6 @@
>   return rv;
> }
> 
>-
>-static nsresult
>-AddPluginInfoToRegistry(nsIRegistry* registry, nsRegistryKey top,
>-                        nsPluginTag *tag, const char *keyname)
>-{
>-  NS_ENSURE_ARG_POINTER(tag);
>-  nsRegistryKey pluginKey;
>-  nsresult rv = registry->AddSubtree(top, keyname, &pluginKey);
>-  if (NS_FAILED(rv)) return rv;
>-
>-  // Add filename, name and description
>-  registry->SetStringUTF8(pluginKey, kPluginsFilenameKey, tag->mFileName);
>-  if (tag->mFullPath)
>-    registry->SetStringUTF8(pluginKey, kPluginsFullpathKey, tag->mFullPath);
>-
>-  // we use SetBytes instead of SetString to address special character issue, see Mozilla bug 108246
>-  if(tag->mName)
>-    registry->SetBytesUTF8(pluginKey, kPluginsNameKey, strlen(tag->mName) + 1, (PRUint8 *)tag->mName);
>-  
>-  if(tag->mDescription)
>-    registry->SetBytesUTF8(pluginKey, kPluginsDescKey, strlen(tag->mDescription) + 1, (PRUint8 *)tag->mDescription);
>-
>-  registry->SetLongLong(pluginKey, kPluginsModTimeKey, &(tag->mLastModifiedTime));
>-  registry->SetInt(pluginKey, kPluginsCanUnload, tag->mCanUnloadLibrary);
>-
>-  // Add in each mimetype this plugin supports
>-  for (int i=0; i<tag->mVariants; i++) {
>-    char mimetypeKeyName[16];
>-    nsRegistryKey mimetypeKey;
>-    PR_snprintf(mimetypeKeyName, sizeof (mimetypeKeyName), "mimetype-%d", i);
>-    rv = registry->AddSubtree(pluginKey, mimetypeKeyName, &mimetypeKey);
>-    if (NS_FAILED(rv))
>-      break;
>-    registry->SetStringUTF8(mimetypeKey, kPluginsMimeTypeKey, tag->mMimeTypeArray[i]);
>-
>-    if(tag->mMimeDescriptionArray && tag->mMimeDescriptionArray[i])
>-      registry->SetBytesUTF8(mimetypeKey, kPluginsMimeDescKey, 
>-                             strlen(tag->mMimeDescriptionArray[i]) + 1, 
>-                             (PRUint8 *)tag->mMimeDescriptionArray[i]);
>-
>-    registry->SetStringUTF8(mimetypeKey, kPluginsMimeExtKey, tag->mExtensionsArray[i]);
>-  }
>-  if (NS_FAILED(rv))
>-    rv = registry->RemoveSubtree(top, keyname);
>-
>-  return rv;
>-}
>-
>-
> ////////////////////////////////////////////////////////////////////////
> nsresult
> nsPluginHostImpl::LoadXPCOMPlugins(nsIComponentManager* aComponentManager)
>@@ -5594,124 +5571,293 @@
>   return NS_OK;
> }
> 
>-
> nsresult
>-nsPluginHostImpl::LoadCachedPluginsInfo(nsIRegistry* registry)
>+nsPluginHostImpl::WritePluginInfo()
> {
>-  // Loads cached plugin info from registry
>-  //
>-  // Enumerate through that list now, creating an nsPluginTag for
>-  // each.
>-  if (! registry)
>-    return NS_ERROR_FAILURE;
> 
>-  nsRegistryKey pluginsKey;
>-  nsresult rv = registry->GetSubtree(nsIRegistry::Common, kPluginsRootKey, &pluginsKey);
>-  if (NS_FAILED(rv)) return rv;
>+  nsresult rv = NS_OK;
>+  nsCOMPtr<nsIProperties> directoryService(do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID,&rv));
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-  // Make sure we are dealing with the same version number of the plugin info
>-  nsXPIDLCString version;
>-  rv = registry->GetStringUTF8(pluginsKey, kPluginsVersionKey, getter_Copies(version));
>-  if (NS_FAILED(rv) || PL_strcmp(version.get(), kPluginInfoVersion)) {
>-    // Version mismatch
>-    // Nuke the subtree
>-    registry->RemoveSubtree(nsIRegistry::Common, kPluginsRootKey);
>-    PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_BASIC,
>-           ("LoadCachedPluginsInfo : Version %s mismatch - Expected %s. Nuking cached info.\n",
>-            version.get(), kPluginInfoVersion));
>-    return NS_ERROR_FAILURE;
>-  }
>+  directoryService->Get(NS_APP_APPLICATION_REGISTRY_DIR, NS_GET_IID(nsIFile), 
>+                        getter_AddRefs(mPluginsDir));
> 
>-  // XXX get rid nsIEnumerator someday!
>-  nsCOMPtr<nsIEnumerator> enumerator;
>-  rv = registry->EnumerateSubtrees(pluginsKey, getter_AddRefs(enumerator));
>-  if (NS_FAILED(rv)) return rv;
>+  if (!mPluginsDir)
>+    return NS_ERROR_FAILURE;
> 
>-  nsCOMPtr<nsISimpleEnumerator> plugins;
>-  rv = NS_NewAdapterEnumerator(getter_AddRefs(plugins), enumerator);
>-  if (NS_FAILED(rv)) return rv;
>+  PRFileDesc* fd = nsnull;
>+ 
>+  nsCOMPtr<nsIFile> pluginReg;
> 
>-  for (;;) {
>-    PRBool hasMore;
>-    plugins->HasMoreElements(&hasMore);
>-    if (! hasMore)
>-      break;
>+  rv = mPluginsDir->Clone(getter_AddRefs(pluginReg));       
>+  if (NS_FAILED(rv))
>+    return rv;
>+ 
>+  rv = pluginReg->AppendNative(kPluginRegistryFilename);
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-    nsCOMPtr<nsISupports> isupports;
>-    plugins->GetNext(getter_AddRefs(isupports));
>+  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(pluginReg, &rv);
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-    nsCOMPtr<nsIRegistryNode> node = do_QueryInterface(isupports);
>-    NS_ASSERTION(node != nsnull, "not an nsIRegistryNode");
>-    if (! node)
>-      continue;
>+  rv = localFile->OpenNSPRFileDesc(PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE, 0600, &fd);
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-    // Pull out the information for an individual plugin, and link it
>-    // in to the mCachedPlugins list.
>-    nsRegistryKey key;
>-    node->GetKey(&key);
>+  PR_fprintf(fd, "Generated File. Do not edit.\n");
> 
>-    nsPluginTag* tag = nsnull;
>-    rv = LoadXPCOMPlugin(registry, nsnull, key, &tag);
>-    if (NS_FAILED(rv))
>-      continue;
>+  PR_fprintf(fd, "\n[HEADER]\nVersion%c%d%c%d\n",
>+             PLUGIN_REGISTRY_FIELD_DELIMITER,
>+             PLUGIN_REGISTRY_VERSION_MAJOR,
>+             PLUGIN_REGISTRY_FIELD_DELIMITER,
>+             PLUGIN_REGISTRY_VERSION_MINOR);
> 
>-    tag->SetHost(this);
>+  // Store all plugins in the mPlugins list - all plugins currently in use.
>+  PR_fprintf(fd, "\n[PLUGINS]");
> 
>-    // Mark plugin as loaded from cache
>-    tag->Mark(NS_PLUGIN_FLAG_FROMCACHE);
>-    PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_BASIC,
>-           ("LoadCachedPluginsInfo : Loading Cached plugininfo for %s\n", tag->mFileName));
>-    tag->mNext = mCachedPlugins;
>-    mCachedPlugins = tag;
>+  nsPluginTag *taglist[] = {mPlugins,mCachedPlugins};
>+  for (int i=0; i<sizeof(taglist)/sizeof(nsPluginTag *); i++) {
>+    for (nsPluginTag *tag = taglist[i]; tag; tag=tag->mNext) {
>+      // store each plugin info into the registry
>+      //filename|fullpath|lastModifiedTimeStamp|canUnload|tag->mFlags
>+      PR_fprintf(fd, "\n%s%c%s%c%lld%c%d%c%lu\n",
>+        (tag->mFileName ? tag->mFileName : ""),
>+        PLUGIN_REGISTRY_FIELD_DELIMITER,
>+        (tag->mFullPath ? tag->mFullPath : ""),
>+        PLUGIN_REGISTRY_FIELD_DELIMITER,
>+        tag->mLastModifiedTime,PLUGIN_REGISTRY_FIELD_DELIMITER,
>+        tag->mCanUnloadLibrary,PLUGIN_REGISTRY_FIELD_DELIMITER,
>+        tag->mFlags);
>+      
>+      //description\n  --\  on separate line, so there is no need to parse it
>+      //name\n         --/  and check for delimiters
>+      //mtypecount
>+      PR_fprintf(fd, "%s\n%s\n%d\n", 
>+        tag->mDescription,
>+        tag->mName,
>+        tag->mVariants);
>+      
>+      // Add in each mimetype this plugin supports
>+      for (int i=0; i<tag->mVariants; i++) {
>+        PR_fprintf(fd, "%d%c%s%c%s%c%s\n",
>+          i,PLUGIN_REGISTRY_FIELD_DELIMITER,
>+          tag->mMimeTypeArray[i],PLUGIN_REGISTRY_FIELD_DELIMITER,
>+          tag->mMimeDescriptionArray[i],PLUGIN_REGISTRY_FIELD_DELIMITER,
>+          tag->mExtensionsArray[i]);
>+      }
>+    }
>   }
> 
>+  if (fd)
>+    PR_Close(fd);
>   return NS_OK;
> }
> 
> nsresult
>-nsPluginHostImpl::CachePluginsInfo(nsIRegistry* registry)
>+nsPluginHostImpl::ReadPluginInfo()
> {
>-  // Save plugin info from registry
>+  nsManifestLineReader reader;
>+  nsresult rv;
> 
>-  if (! registry)
>-    return NS_ERROR_FAILURE;
>+  nsCOMPtr<nsIProperties> directoryService(do_GetService(NS_DIRECTORY_SERVICE_CONTRACTID,&rv));
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-  // We don't want any old plugins that don't exist anymore hanging around
>-  // So remove and re-add the root of the plugins info
>-  nsresult rv = registry->RemoveSubtree(nsIRegistry::Common, kPluginsRootKey);
>-  
>-  nsRegistryKey pluginsSubtreeKey;
>-  rv = registry->AddSubtree(nsIRegistry::Common, kPluginsRootKey, &pluginsSubtreeKey);
>-  if (NS_FAILED(rv)) return rv;
>+  directoryService->Get(NS_APP_APPLICATION_REGISTRY_DIR, NS_GET_IID(nsIFile),
>+                        getter_AddRefs(mPluginsDir));
>+   
>+  if (!mPluginsDir)
>+    return NS_ERROR_FAILURE;  
> 
>-  rv = registry->SetStringUTF8(pluginsSubtreeKey, kPluginsVersionKey, kPluginInfoVersion);
>-  if (NS_FAILED(rv)) return rv;
>+  PRFileDesc* fd = nsnull;
>+ 
>+  nsCOMPtr<nsIFile> pluginReg;
>+ 
>+  rv = mPluginsDir->Clone(getter_AddRefs(pluginReg));
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-  int count = 0;
>+  rv = pluginReg->AppendNative(kPluginRegistryFilename);
>+  if (NS_FAILED(rv))
>+    return rv;
> 
>-  // Store all plugins in the mPlugins list - all plugins currently in use.
>-  char pluginKeyName[64];
>-  nsPluginTag *tag;
>-  for(tag = mPlugins; tag; tag=tag->mNext) {
>-    // store each plugin info into the registry
>-    PR_snprintf(pluginKeyName, sizeof(pluginKeyName), "plugin-%d", ++count);
>-    AddPluginInfoToRegistry(registry, pluginsSubtreeKey, tag, pluginKeyName);
>+  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(pluginReg, &rv);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  rv = localFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &fd);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  PRInt64 fileSize;
>+  rv = localFile->GetFileSize(&fileSize);
>+  if (NS_FAILED(rv)) {
>+    PR_Close(fd);
>+    return rv;
>+  }
>+
>+  PRInt32 flen = nsInt64(fileSize);
>+  if (flen == 0) {
>+    PR_Close(fd);
>+    NS_WARNING("Plugins Registry Empty!");
>+    return NS_OK; // ERROR CONDITION
>   }
>+ 
>+  // set rv to return an error on goto out 
>+  rv = NS_ERROR_FAILURE;
> 
>-  // Store any unwanted plugins info so that we wont have to load
>-  // them again the next time around
>-  tag = mCachedPlugins;
>-  for(tag = mCachedPlugins; tag; tag=tag->mNext) {
>-    // Look for unwanted plugins
>-    if (!(tag->mFlags & NS_PLUGIN_FLAG_UNWANTED))
>+  char* registry = new char[flen+1];
>+  if (!registry) {
>+    goto out;
>+  }
>+    
>+  if (flen > PR_Read(fd, registry, flen)) {
>+    goto out;
>+  }
>+
>+  registry[flen] = '\0';
>+    
>+  reader.Init(registry, flen);
>+   
>+  if (!ReadSectionHeader(reader, "HEADER")) {
>+    goto out;
>+  }
>+
>+  if (!reader.NextLine()) {
>+    goto out;
>+  }
>+
>+  char* values[6]; 
>+    
>+  // VersionLiteral,major,minor
>+  if (3 != reader.ParseLine(values, 3)) {
>+    goto out;
>+  }
>+    
>+  // VersionLiteral
>+  if (PL_strcmp(values[0], "Version")) {
>+    goto out;
>+  }
>+   
>+  // major
>+  if (PLUGIN_REGISTRY_VERSION_MAJOR != atoi(values[1])) {
>+    goto out;
>+  }
>+   
>+  // minor
>+  if (PLUGIN_REGISTRY_VERSION_MINOR != atoi(values[2])) {
>+    goto out;
>+  }
>+
>+  if (!ReadSectionHeader(reader, "PLUGINS")) {
>+    goto out;
>+  }
>+
>+  while (1) {
>+
>+    if(!reader.NextLine())
>+      goto out;
>+       
>+    //filename|fullpath|lastModifiedTimeStamp|canUnload|tag.mFlag
>+    if (5 != reader.ParseLine(values, 5))
>+      break;
>+
>+    char *filename = values[0];
>+    char *fullpath = values[1];
>+    PRInt64 lastmod = nsCRT::atoll(values[2]);
>+    PRBool canunload = atoi(values[3]);
>+    PRUint32 tagflag = atoi(values[4]);
>+
>+    //description is whole line and can contain any chars
>+    if (!reader.NextLine())
>+      goto out;
>+    char *description = reader.LinePtr();
>+
>+    //name is whole line and can contain any chars
>+    if (!reader.NextLine())
>+      goto out;
>+    char *name = reader.LinePtr();
>+
>+    //mimetypecount
>+    if (!reader.NextLine())
>+      goto out;    
>+    int mimetypecount = atoi(reader.LinePtr());
>+
>+    char *stackalloced[PLUGIN_REGISTRY_MAX_MIMETYPES_PER_PLUGIN * 3];
>+    char **mimetypes;
>+    char **mimedescriptions;
>+    char **extensions;
>+    char **heapalloced = 0;
>+    if (mimetypecount > PLUGIN_REGISTRY_MAX_MIMETYPES_PER_PLUGIN - 1) {
>+      heapalloced = new char *[mimetypecount * 3];
>+      mimetypes = heapalloced;
>+    } else {
>+      mimetypes = stackalloced;
>+    }
>+    mimedescriptions = mimetypes + mimetypecount;
>+    extensions = mimedescriptions + mimetypecount;
>+
>+    int mtr = 0; //mimetype read
>+    for (; mtr < mimetypecount; mtr++) {
>+      if (!reader.NextLine())
>+        break;
>+           
>+      //line number|mimetype|description|extension
>+      if (4 != reader.ParseLine(values, 4))
>+        break;
>+      int line = atoi(values[0]);
>+      if (line != mtr)
>+        break; 
>+      mimetypes[mtr] = values[1];
>+      mimedescriptions[mtr] = values[2];
>+      extensions[mtr] = values[3];
>+    }
>+
>+    if (mtr != mimetypecount) {
>+      if (heapalloced) {
>+        delete [] heapalloced;
>+      }
>+      goto out;
>+    }
>+    
>+    nsPluginTag* tag = new nsPluginTag(name,
>+                                       description,
>+                                       filename,
>+                                       (*fullpath ? fullpath : 0), // we have to pass 0 prt if it's empty str
>+                                       (const char* const*)mimetypes,
>+                                       (const char* const*)mimedescriptions,
>+                                       (const char* const*)extensions,
>+                                       mimetypecount, lastmod, canunload);
>+    if (heapalloced) {
>+        delete [] heapalloced;
>+    }
>+
>+    if (!tag) {
>       continue;
>+    }
>+
>+    // Mark plugin as loaded from cache
>+    tag->Mark(NS_PLUGIN_FLAG_FROMCACHE);
>+    PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_BASIC,
>+           ("LoadCachedPluginsInfo : Loading Cached plugininfo for %s\n", tag->mFileName));
>+    tag->mFlags = tagflag;
>+    tag->mNext = mCachedPlugins;
>+    mCachedPlugins = tag;
> 
>-    // store each plugin info into the registry
>-    PR_snprintf(pluginKeyName, sizeof(pluginKeyName), "plugin-%d", ++count);
>-    AddPluginInfoToRegistry(registry, pluginsSubtreeKey, tag, pluginKeyName);
>   }
> 
>-  return NS_OK;
>+  rv = NS_OK;
>+
>+out:
>+  if (fd)
>+    PR_Close(fd);
>+    
>+  if (registry)
>+    delete [] registry;
>+
>+  return rv;
> }
> 
> nsPluginTag *
>Index: nsPluginHostImpl.h
>===================================================================
>RCS file: /cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.h,v
>retrieving revision 1.83
>diff -u -r1.83 nsPluginHostImpl.h
>--- nsPluginHostImpl.h	14 Jul 2002 00:39:35 -0000	1.83
>+++ nsPluginHostImpl.h	22 Jul 2002 20:07:20 -0000
>@@ -434,11 +434,11 @@
> 
>   PRBool IsRunningPlugin(nsPluginTag * plugin);
> 
>-  // Loads all cached plugins info into mCachedPlugins
>-  nsresult LoadCachedPluginsInfo(nsIRegistry* registry);
>-
>   // Stores all plugins info into the registry
>-  nsresult CachePluginsInfo(nsIRegistry* registry);
>+  nsresult WritePluginInfo();
>+
>+  // Loads all cached plugins info into mCachedPlugins
>+  nsresult ReadPluginInfo();
> 
>   // Given a filename, returns the plugins info from our cache
>   // and removes it from the cache.
>@@ -473,6 +473,7 @@
>   nsActivePluginList mActivePluginList;
>   nsVoidArray mUnusedLibraries;
> 
>+  nsCOMPtr<nsIFile> mPluginsDir;
>   nsCOMPtr<nsIDirectoryServiceProvider> mPrivateDirServiceProvider;  
>   nsWeakPtr mCurrentDocument; // weak reference, we use it to id document only
> 
>Index: nsPluginManifestLineReader.h
>===================================================================
>RCS file: nsPluginManifestLineReader.h
>diff -N nsPluginManifestLineReader.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ nsPluginManifestLineReader.h	22 Jul 2002 20:07:20 -0000
>@@ -0,0 +1,126 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Netscape Public License
>+ * Version 1.1 (the "License"); you may not use this file except in
>+ * compliance with the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/NPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is 
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or 
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the NPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the NPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+#ifndef nsPluginManifestLineReader_h__
>+#define nsPluginManifestLineReader_h__
>+
>+#include "nspr.h"
>+#include "nsDebug.h"
>+
>+
>+#define PLUGIN_REGISTRY_VERSION_MINOR 6
>+#define PLUGIN_REGISTRY_VERSION_MAJOR 0
>+#ifdef XP_WIN
>+#define PLUGIN_REGISTRY_FIELD_DELIMITER '|'
>+#else
>+#define PLUGIN_REGISTRY_FIELD_DELIMITER ':'
>+#endif
>+
>+#define PLUGIN_REGISTRY_MAX_MIMETYPES_PER_PLUGIN 64
>+
>+class nsManifestLineReader
>+{
>+public:
>+    nsManifestLineReader() : mBase(nsnull) {} 
>+    ~nsManifestLineReader() {}
>+
>+    void Init(char* base, PRUint32 flen) 
>+    {
>+        mBase = mCur = mNext = base; 
>+        mLength = 0;
>+        mLimit = base + flen;
>+    }
>+
>+    PRBool NextLine()
>+    {
>+        if(mNext >= mLimit)
>+            return PR_FALSE;
>+        
>+        mCur = mNext;
>+        mLength = 0;
>+        
>+        while(mNext < mLimit)
>+        {
>+            if(IsEOL(*mNext))
>+            {
>+                *mNext = '\0';
>+                for(++mNext; mNext < mLimit; ++mNext)
>+                    if(!IsEOL(*mNext))
>+                        break;
>+                return PR_TRUE;
>+            }
>+            ++mNext;
>+            ++mLength;
>+        }
>+        return PR_FALSE;        
>+    }
>+
>+    int ParseLine(char** chunks, int maxChunks)
>+    {
>+        NS_ASSERTION(mCur && maxChunks && chunks, "bad call to ParseLine");
>+        int found = 0;
>+        chunks[found++] = mCur;
>+        
>+        if(found < maxChunks)
>+        {
>+            for(char* cur = mCur; *cur; cur++)
>+            {
>+                if(*cur == PLUGIN_REGISTRY_FIELD_DELIMITER)
>+                {
>+                    *cur = 0;
>+                    chunks[found++] = cur+1;
>+                    if(found == maxChunks)
>+                        break;
>+                }
>+            }
>+        }
>+        return found;
>+    }
>+
>+    char*       LinePtr() {return mCur;}    
>+    PRUint32    LineLength() {return mLength;}    
>+
>+    PRBool      IsEOL(char c) {return c == '\n' || c == '\r';}
>+private:
>+    char*       mCur;
>+    PRUint32    mLength;
>+    char*       mNext;
>+    char*       mBase;
>+    char*       mLimit;
>+};
>+
>+#endif

Comment 91

[serge (gone)]

Attachment: patch v3

it addresses dveditz's concerns, except  "plugin cache format needs escaping",
as I said above '|' is mimetype descriptions delimiter on win32 as well as ':'
is unix/mac delimiter and current plugin implementation does not support
multibyte charset for mimetypes.
Thanks av for NS_PLUGIN_FLAG_UNWANTED addon.
BTW:
+ for (nsPluginTag *tag = taglist[i]; tag; tag=tag->mNext) {
does the right thing if mPlugins or mCachedPlugins == 0, so there's no need to
check it somewhere else

Comment 92

[serge (gone)]

Attachment: patch v4 addresses brendan comments

Comment 93

[Brendan Eich [:brendan]]

Comment on attachment 92320 [details] [diff] [review]
patch v4 addresses brendan comments

D'oh!  I thought I was clearly favoring a do-while loop (do {...} while
(reader.NextLine()); to be precise), instead of  either while (1){...; if
(!reader.NextLine()) break;} or for (;;){...; if (!reader.NextLine()) break;})
as dveditz suggested.  There is no reason to obfuscate the control flow here.

/be

Comment 94

[serge (gone)]

Attachment: patch v5 with do {} while() in ReadSectionHeader()

Comment 95

[Brendan Eich [:brendan]]

Comment on attachment 92326 [details] [diff] [review]
patch v5 with do {} while() in ReadSectionHeader()

>+  do {
>+    if (*reader.LinePtr() == '[') {
>+      char* p = reader.LinePtr() + (reader.LineLength() - 1);
>+      if (*p != ']')
>+        break;
>+      *p = 0;
>+      
>+      char* values[1];
>+      if (1 != reader.ParseLine(values, 1))
>+        break;
>+      // ignore the leading '['
>+      if (0 != PL_strcmp(values[0]+1, token)) {
>+        if (!reader.NextLine())
>+          break;
>+        else
>+          continue;
>+      }
>+      return PR_TRUE;
>+    }
>+  } while (reader.NextLine());

First, else after break is unnecessary and was in the old patch -- but second,
and more important, now that you've moved the reader.NextLine() into the loop
control, the whole if-else:

+	 if (!reader.NextLine())
+	   break;
+	 else
+	   continue;

is wrong.  It will skip an extra line until end of file.  All you need is:

+      if (0 != PL_strcmp(values[0]+1, token))
+	 continue;

As a matter of style, I see no point in the 0 != here, but I'd better shut up. 
One more patch, please, and read the whole loop yourself to be convinced that
it's right (I wasn't posing a trick question when I seconded dveditz's call for
the do-while loop, honest!  I would hate to let a bug get introduced by such a
change, though).

/be

Comment 96

[serge (gone)]

Attachment: patch v6 with correction in ReadSectionHeader()

Comment 97

[serge (gone)]

actually the code for ReadSectionHeader() was borrowed by neeti from
http://lxr.mozilla.org/seamonkey/source/xpcom/components/nsComponentManager.cpp#957
or from
http://lxr.mozilla.org/seamonkey/source/xpcom/reflect/xptinfo/src/xptiManifest.cpp#348

Comment 98

[Brendan Eich [:brendan]]

Comment on attachment 92332 [details] [diff] [review]
patch v6 with correction in ReadSectionHeader()

Please try mentally executing the code to see that it takes the intended steps,
and/or the same steps as the previous patches -- you need a continue, not a
break, if PL_strcmp (which isn't needed here, strcmp is supported on all
platforms, but there I go again :-) returns non-zero.  It'd be good to see a
tested patch at this point....

/be

Comment 99

[Brendan Eich [:brendan]]

serge: I followed those lxr links, thanks.  Something's clearly amiss, starting
with attachment 80814 [details] [diff] [review] in this bug ("patch1") at least.  That patch and all later
ones till your last, attachment 92332 [details] [diff] [review], *ocntinued* after PL_strcmp returned a
nono-zero result (unless at end of file, i.e., unless NextLine returned false).
 Hence my last comment, and needs-work mark.

Now that I follow the links you gave, I see the code in xpcom breaking, not
continuing, on a mismatch with token.  That could be right for the xpcom cases,
but wrong here -- it depends on how ReadSectionHeader is meant to be called. 
Should it keep on searching for a matching [token...]?  Or should it give up if
the next [...] line does not begin (after the [) with the desired token?

/be

Comment 100

[Brendan Eich [:brendan]]

Argh, fingers tired:

s/*ocntinued*/*continued*/

/be

Comment 101

[serge (gone)]

>it depends on how ReadSectionHeader is meant to be called
Brendan: you are right it depends, and in this particular
nsPluginHostImpl::ReadPluginInfo() implementation I see no pint to continue
searching for section header, if we fail to get exact header from file with
liner headers structure.

Comment 102

[Brendan Eich [:brendan]]

Comment on attachment 92332 [details] [diff] [review]
patch v6 with correction in ReadSectionHeader()

Ok, if you're sure of the section header order, then the change you made to
break rather than continue is ok with me.  I'll defer to dveditz for restamping
sr= here.

/be

Comment 103

[av (gone)]

Comment on attachment 92332 [details] [diff] [review]
patch v6 with correction in ReadSectionHeader()

r=av

Comment 104

[Daniel Veditz [:dveditz]]

Comment on attachment 92332 [details] [diff] [review]
patch v6 with correction in ReadSectionHeader()

sr=dveditz

Comment 105

[Doug Turner (:dougt)]

over to serge who will land this after 1.1b

Comment 106

[serge (gone)]

on the trunk 
/mozilla/modules/plugin/base/src/nsPluginManifestLineReader.h,v
initial revision: 1.1
/mozilla/modules/plugin/base/src/nsPluginHostImpl.h,v 
new revision: 1.85; previous revision: 1.84
/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v
new revision: 1.412; previous revision: 1.411

Comment 107

[Greg K.]

Was it ever determined whether or not this patch will clean up registries that
are already inflated by this problem, or is another bug needed for that? As
mentioned previously in this bug, it also contains profile data, so simply
deleting it isn't a good option.

Comment 108

[Matthias Versen [:Matti]]

Can can delete the file, run mozilla (and the profilemanager will comes up with
no profiles) and create the profile again with the EXACT same name.
Mozilla will use the old profile data (works on linux and win32)

Comment 109

[serge (gone)]

no, this patch (attachment 92332 [details] [diff] [review]) does not implement registry Pack(),
and registry.dat remains untouched.

Comment 110

[Greg K.]

FWIW, it appears comment 26 refers to the registry packing implementation bug.

Comment 111

[han.holl]

I don't know if this bug is resolved.
I'm running 1.1, and (see comment 107) appreg does not
get smaller (about 550k here), and deleting it
(as in comment 108) no longer works:
I get 'A profile with the name han already exists,
please use another name' os somthing very like it.
If I let mozilla use my existing registry, it says it
has to convert, and I lose my signon file and wallet.
I really cannot remember all those passwords mtself !

Comment 112

[serge (gone)]

ham, I did deleting registry an recreating it again several times w/o any problem,
if you afraid to lose the data make a back up copy of it

Comment 113

[WD]

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

Comment 114

[Paul Wyskoczka]

Updating QA contact to [e-mail]