Closed Bug 109739 Opened 23 years ago Closed 22 years ago

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

Categories

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

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: spam, Assigned: srgchrpv)

References

Details

(Keywords: perf, topembed, Whiteboard: [ADT2] [needs reviews])

Attachments

(1 file, 12 obsolete files)

patch v6 with correction in ReadSectionHeader()
23.86 KB, patch
serhunt
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
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.
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.)
reading that file can't be good for startup performance. :)
Keywords: perf
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.
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.
Severity: normal → minor
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.
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.
--- Mass reassigning Unix bugs to serge ---
Assignee: av → serge
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.
Assignee: serge → dveditz
OS: Linux → All
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.
Assignee: dveditz → serge
>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?
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.
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.
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.
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.
--->dougt says he wants this
Assignee: serge → dougt
Attached patch Key plugin by its given name. (obsolete) — — Splinter Review
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).
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.
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 on attachment 69581 [details] [diff] [review] Key plugin by its given name. r=av given dup plugins are reflected.
Attachment #69581 - Flags: review+
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.
- // 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 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?
Attachment #69581 - Flags: needs-work+
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.
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.
Attached patch Using Pack() (obsolete) — — Splinter Review
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.
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?).
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.
dan, can you post a patch which fixes up these call sites and does what you are suggesting?
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?
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).
Dan, if I convert the two callers of SetStringUTF8 to SetBytesUTF8, will that break existing clients?
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.
passing to neeti.
Assignee: dougt → neeti
Target Milestone: --- → mozilla1.0
shouldn't this be also done for the mozilla0.9.9 branch?
Neeti, what is the status of this bug?
Keywords: mozilla1.0, nsbeta1, topembed
Priority: -- → P3
[adt needinfo] what's the net effect of this? How risky is the fix? What is the fix... what is the meaning of life? :)
Whiteboard: [adt needinfo]
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.
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
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.
plusing this bug as per adt plugins triage.
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt needinfo] → [adt2]
topembed+ per EDT triage.
Keywords: topembed → topembed+
*** Bug 131851 has been marked as a duplicate of this bug. ***
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.
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 ?
Attached patch patch1 (obsolete) — — Splinter Review
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 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.
Attachment #80814 - Flags: needs-work+
Attached patch revised patch (obsolete) — — Splinter Review
Attached a revised patch for review.
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?
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.
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.)
>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 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.
Attachment #82536 - Flags: review+
*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.
Keywords: nsbeta1+, topembed+ → nsbeta1, topembed
Whiteboard: [adt2]
Attachment #82536 - Flags: needs-work+
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.
Keywords: topembed → topembed-
*** Bug 146072 has been marked as a duplicate of this bug. ***
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.
*** Bug 150221 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.1
*** Bug 150344 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.0 → mozilla1.0.1
*** Bug 130322 has been marked as a duplicate of this bug. ***
*** Bug 157627 has been marked as a duplicate of this bug. ***
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.
Like you're saying, this seems to block bug 157627. Normal severity. Seems like a high priority bug.
Blocks: 157627
Severity: minor → normal
Whiteboard: [Impact Needed]
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
Keywords: nsbeta1, topembed- → nsbeta1+, topembed
Whiteboard: [Impact Needed] → [ADT1 RTM] [ETA Needed]
Target Milestone: mozilla1.0 → mozilla1.0.1
neeti is out. I will take. You just want this fixed on the TRUNK right?
Assignee: neeti → dougt
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
Keywords: nsbeta1+, topembed → nsbeta1, topembed-
Whiteboard: [ADT1 RTM] [ETA Needed] → [Impact Needed]
Target Milestone: mozilla1.0.1 → mozilla1.0
Doug: I would suspect that we need this fixed on the trunk and the branch
Keywords: nsbeta1, topembed- → nsbeta1+, topembed
Whiteboard: [Impact Needed] → [ADT1 RTM] [ETA Needed]
Target Milestone: mozilla1.0 → mozilla1.0.1
Attached patch update of neeti's work (obsolete) — — Splinter Review
Attachment #69581 - Attachment is obsolete: true
Attachment #69614 - Attachment is obsolete: true
Attachment #80814 - Attachment is obsolete: true
Attachment #81738 - Attachment is obsolete: true
Attachment #82536 - Attachment is obsolete: true
Attached patch xpcom part of the patch (obsolete) — — Splinter Review
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.
Doug, last two patches are identical, and there is no xpcom part
by xpcom part i mean changes in nsPluginHostImpl::LoadXPCOMPlugins()
Attached patch slightly modified doug's patch (obsolete) — — Splinter Review
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.
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.
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.
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?).
Blocks: profile-corrupt
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.
bug 142247 is a fine sample of what can happen because moz wants to write to appreg all the time.
dugt: Can we at least pursue reviews and get this landed on the trunk asap, just in case?
Whiteboard: [ADT1 RTM] [ETA Needed] → [ADT1 RTM] [ETA 07/24] [needs reviews]
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.
so which attachment should I be reviewing? I see 3 non-obsolete patches here.
Doug I've marked all prev patches as obsolete, to avoid confusions. Alec, would you sr= on this patch, please
Attachment #92002 - Attachment is obsolete: true
Attachment #92008 - Attachment is obsolete: true
Attachment #92072 - Attachment is obsolete: true
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.
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 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 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 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.
Attachment #92267 - Flags: needs-work+
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.
Changing to adt2 per the adt.
Whiteboard: [ADT1 RTM] [ETA 07/24] [needs reviews] → [ADT2] [needs reviews]
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
Attached patch patch v3 (obsolete) — — Splinter Review
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
Attachment #92267 - Attachment is obsolete: true
Attachment #92311 - Attachment is obsolete: true
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
Attachment #92320 - Attachment is obsolete: true
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
Attachment #92326 - Flags: needs-work+
Attachment #92326 - Attachment is obsolete: true
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
Attachment #92332 - Flags: needs-work+
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
Argh, fingers tired: s/*ocntinued*/*continued*/ /be
>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 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
Attachment #92332 - Flags: needs-work+
Comment on attachment 92332 [details] [diff] [review] patch v6 with correction in ReadSectionHeader() r=av
Attachment #92332 - Flags: review+
Comment on attachment 92332 [details] [diff] [review] patch v6 with correction in ReadSectionHeader() sr=dveditz
Attachment #92332 - Flags: superreview+
over to serge who will land this after 1.1b
Assignee: dougt → serge
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
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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.
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)
no, this patch (attachment 92332 [details] [diff] [review]) does not implement registry Pack(), and registry.dat remains untouched.
FWIW, it appears comment 26 refers to the registry packing implementation bug.
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 !
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
*** Bug 179389 has been marked as a duplicate of this bug. ***
Updating QA contact to bmartin@netscape.com
QA Contact: shrir → bmartin
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: