Closed Bug 159445 Opened 22 years ago Closed 21 years ago

Scan for PLIDs upon startup or load time (for plugins)

Categories

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

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.4final

People

(Reporter: arun, Assigned: peterlubczynski-bugs)

References

Details

(Keywords: fixed1.4)

Attachments

(1 file, 2 obsolete files)

Bug 44973 makes mention of PLIDs in tags like OBJECT (and perhaps EMBED) that
gives plugin authors a way to invoke plugins uniquely, without fear of MIME type
collisions among popular types.  But this bug raises a different aspect of PLIDs
on Windows.  Before mentioning the bug, the "specifications" are:

http://mozilla.org/projects/plugins/first-install-problem.html and
http://mozilla.org/projects/plugins/plugin-identifier.html .

PLIDs on Windows are registry entries in the Win32 System Registry.  When the
browser scans for plugins in its own plugins directories ("plugins" directory
for Mozilla, for example), it should also add the plugins directories specified
under the PLID keys.  In other words, it should also scan for plugins that were
installed into separate directories before Gecko-based browser even arrived on
the desktop.  It should scan HKEY_LOCAL_MACHINE and the MozillaPlugins subkey,
then look for the Path and XPTPath strings, and make sure it knows where to find
these components.  It can also build MIME type information from the other
strings written to the registry in the spec.
(http://mozilla.org/projects/plugins/first-install-problem.html)
The distinction: Bug 44973 mentions tags such as OBJECT using PLID, but this bug
specifically mentions a startup scan of the registry to add further plugins
directories to the list of existing plugins directories.
Priority: -- → P2
Whiteboard: [PL2:P2][Plid][Plug-in Mgr]
Target Milestone: --- → mozilla1.2beta
Depends on: 184506
moving to 1.4 beta : plugin branch work
Assignee: beppe → anthonyd
Whiteboard: [PL2:P2][Plid][Plug-in Mgr] → [PL:BRANCH][Plid][Plug-in Mgr]
Target Milestone: mozilla1.2beta → mozilla1.4beta
*** Bug 164980 has been marked as a duplicate of this bug. ***
--->peterl
Assignee: anthonyd → peterlubczynski
Blocks: 44973
Target Milestone: mozilla1.4beta → Future
Status: NEW → ASSIGNED
QA Contact: shrir → bmartin
Attached patch patch v.1 (obsolete) — Splinter Review
okay, here's a patch that finds plugins based on the information left in their
PLID Windows Registry key. I've tested with Real and Viewpoint and both seemed
to be correctly "scanned". There are several limitation:

1) You either get ALL the plugins specified by PLIDs or none. Users or
embedders can toggle this with the new pref in winpref.js: plugin.scan.plid.all
(default is true)

2) XPT paths doesn't work. Bug 44973 is blocking this and looks like a lot of
work.

3) Any plugins found in the directory specified by the Path key will be found.
If the Path key points to a filename, the entire directory will still be
scanned. Our system currently only has the ability to scan a whole directory
vs. just one DLL. For example, when Real One is scanned, it not only picks up
nppl3260.dll which is only what the Path key specifies, but also nprjplug.dll
and nprpjplug.dll which are in the same directory.

4) PLID and their Path keys are scanned in the order they appear in the Windows
registry.

5) PLID Path scanning happens after embedder specified directories but before
scanning teir1 plugin directories (like QT, WMP, Acrobat, Java, etc..). This is
important for dups and resolving mime-type conflicts.

6) Other information from the PLID key, like mime type, is completely ignored.
All the mime and description information is read directly from the DLL. Plugins
will be choosen based on the order in which the mime type appears in
about:plugins.
Whiteboard: [PL:BRANCH][Plid][Plug-in Mgr]
Attached patch patch v.1.1 (obsolete) — Splinter Review
patch ready for reviews
Attachment #122169 - Attachment is obsolete: true
Attachment #123188 - Flags: superreview?(alecf)
Attachment #123188 - Flags: review?(dougt)
Comment on attachment 123188 [details] [diff] [review]
patch v.1.1

+	     for (PRInt32 i = 0; i < dirs.Count(); i++) {

don't call Count() every time - save the value and reuse it.

+	       nsCOMPtr<nsIFile> dup (do_QueryInterface(dirs[i]));

why do_QueryInterface? this should work without even casting, no? (or
NS_STATIC_CAST if you must) - then "dup" can be a raw nsIFile, and you can
avoid the extra addref/release

+  mPrefService = do_GetService(NS_PREFSERVICE_CONTRACTID);
+  if (mPrefService) {

what's the point of caching the prefs service if you're going to call
do_GetService() every time :) Make sure you'd not leaking the pref service
though.

In any case, is it really even worth caching the whole service? you should only
cache it if you expect to be calling methods on it often - this looks like
initialization, mostly (except perhaps that security.enable_java?)

   // This table controls the order of scanning
   const char *prefs[] = {NS_WIN_JRE_SCAN_KEY,	       nsnull,
			  NS_WIN_ACROBAT_SCAN_KEY,     nsnull,

this isn't part of your patch, but if you feel like making this "const char*
const prefs[]" I'll sr= that too :)


sr-'ing this for now - nothing major but there were enough changes to recommend
a new patch.
Attachment #123188 - Flags: superreview?(alecf)
Attachment #123188 - Flags: superreview-
Attachment #123188 - Flags: review?(dougt)
Attached patch patch v.1.2Splinter Review
okay, corrected the patch according to comments....

I left in mPrefService because the old code would fetch it every time whereas
this patch will cache it in the plugin host service constructor and release it
when the service is told to destory (when Notify/Observe(quit-application)
happens). The method I'm adding the new pref check to is |FindPlugins| which is
called frequently enough to cache the service, plus the prefs service is
fetched multiple times in scanning and starting plugins anyway. The
DOMPluginImpl::GetFilename method still call the service every time but I'm
looking forward to re-writing this code in the next milestone anyway.
Attachment #123188 - Attachment is obsolete: true
Attachment #123213 - Flags: superreview?(alecf)
Attachment #123213 - Flags: review?(dougt)
Comment on attachment 123213 [details] [diff] [review]
patch v.1.2

ok, this all looks good except usually we like to release cached services
during xpcom shutdown (NS_XPCOM_SHUTDOWN_OBSERVER_ID rather than
quit-application - xpcom-shutdown is a better-defined topic (i.e. its kind of a
frozen topic, rather than quit-application which may or may not fire in
embedded environments)

other than that, sr=alecf.. thanks for cleaning up the pref table
Attachment #123213 - Flags: superreview?(alecf) → superreview+
Attachment #123213 - Flags: review?(dougt) → review?(jkeiser)
*** Bug 154890 has been marked as a duplicate of this bug. ***
Comment on attachment 123213 [details] [diff] [review]
patch v.1.2

Patch is simpler than it looks :) r=me
Attachment #123213 - Flags: review?(jkeiser) → review+
Comment on attachment 123213 [details] [diff] [review]
patch v.1.2

seeing approval from drivers: this fix is needed so that mozilla can find
plugins that were installed before the browser
Attachment #123213 - Flags: approval1.4?
Keywords: nsbeta1
Comment on attachment 123213 [details] [diff] [review]
patch v.1.2

a=asa (on behalf of drivers) for checkin to 1.4.
Attachment #123213 - Flags: approval1.4? → approval1.4+
I've opened bug 207306 to track cleaning up all cached services in the plugin
host on the correct observer topic.

patch checked into trunk and 1.4 branch, marking FIXED.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla1.4final
Keywords: fixed1.4
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: