154272 - plugins needs to cause XPTI to be refreshed

Status: VERIFIED FIXED

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

Description

[Peter Lubczynski]

This is from bug 134448:

Problem:
Newly dropped XPT files are not picked up automatically at startup

Steps to reproduce (see me or Shrir if you need the Flash6 scriptable beta):
1. Install CLEAN. Start the browser once to ensure xpti.dat gets fully
generated. Exit.
2. Copy scriptable plugin (for example, Flash 6's npswf32.dll) to plugins folder
3. Copy flashplayer.xpt to plugins folder
4. Start browser again. DO NOT VISIT about:plugins.
5. Try a scripting testcase and notice the error in the Javascript console.

Description:
From talking with jband, I found out that directories with XPT files won't be
searched for new files automatically at startup unless:
1. A new directory is added that is not cached in xpti.dat (or the file is
regenerated because it's missing)
2. javascript:navigator.plugins.refresh() is called which forces a componenet
manager auto register hich forces xpti to refresh.

Conclusion:
We really need to automatically pick up new XPT files in plugin directories.
This is very important to solve the "first-install" problem for plugins that are
scriptable. It is too much to ask plugin vendors to find and delete xpit.dat or
run plugins.refresh() and impossible when the browser is not installed.

(The "first-install" problem, to refresh your memory, is when a plugin is
installed before the browser)

Possible ideas for a solution:
Can we just call nsIInterfaceInfoManager->AutoRegisterInterfaces()?
Is this much of a performance hit? Could we simply call it anytime we detect new
plugins have been installed? Could we call it anytime
nsIPluginInsatnce::GetScriptableInterface is called? Thoughts?

Comment 1

[Peter Lubczynski]

Attachment: possible patch?

This patch causes us to scan XTPI the first time plugins (which is currently at
startup when Java is enabled in prefs, see bug 128366) are scanned and when
ReloadPlugins() is called. This patch also wraps full XPCOM component auto
registration which was done on a navigator.plugins.refresh() in a pref and
defaulted to off. I did this because:
1) XPCOM does not manange nsGetFactory plugins anymore
2) We have no teir1 nsIModule plugins
3) We really only wanted XPCOM to refresh XPTI
4) It's a performance hit
5) Get rid of the "no entry point" error sometimes seen in about:plugins

Thoughts?

Comment 2

[Peter Lubczynski]

Attachment: patch v.1.1

...I talk with DougT and it may be better not to change the exisiting behavior
of plugins.refresh() causing XPCOM autoregister as there could possibly be
people depending on this behavior. This is the same patch as before, minus the
changes to DOM.

Comment 3

[rubydoo123]

making this ADT2 and nsbeta1+
who this affects: plug-in vendors and end users

what currently happens: the vendor who uses native installers must delete the
xpti.dat file to ensure that we pick up the newly installed xpt file
what this patch will do: this patch makes us refresh the xpti info whenever we
scan for plug-ins

current scenario: this prevents sriptability from automatically working. In
addition, an end user will have situations where a plug-in would not function
even though the plug-in appears to have been installed properly.

Comment 4

[John Bandhauer]

Please clarify...

Does it call iim->AutoRegisterInterfaces() everytime we scan for plugins? OR
Does it make that call only when new plugins are found when scanning for
plugins? The latter would be much better IMO.

Comment 5

[Peter Lubczynski]

Comment on attachment 89348 [details] [diff] [review]
patch v.1.1

This patch calls AutoRegisterInterfaces() anytime we are asked to detect new
plugins, even if no new ones were found. My thinking was that since XPT files
are new to plugins, sometimes they get orphaned from their plugin and then
getting them picked up might be quite frustrating to the user.

Currently, we are asked to detect new plugins when:
1) whichever comes first, either when OJI is initilized (done at startup if
Java is enabled, see bug 128366) or when the user first comes across a page
that needs a plugin.
2) Once per document if the default plugin is shown
3) When navigator.plugins.refresh() is called

If bug 128366 where fixed, plugins wouldn't need to scan at startup and
therefore all scaning would be postponed until needed.

Comment 6

[av (gone)]

Will just this be not enough?

NS_IMETHODIMP nsPluginHostImpl::LoadPlugins()
{
  // do not do anything if it is already done
  // use ReloadPlugins() to enforce loading
  if(mPluginsLoaded)
    return NS_OK;

  PRBool pluginschanged;
- return FindPlugins(PR_TRUE, &pluginschanged);
+  nresult rv = FindPlugins(PR_TRUE, &pluginschanged);
+  if (NS_FAILED(rv))
+    return rv;
+
+  if (pluginschanged) {
+    // rescan XPTI to catch any newly installed interfaces
+    nsCOMPtr<nsIInterfaceInfoManager> iim (...getter...);
+    if (iim)
+      iim->AutoRegisterInterfaces();
+  }
+  return NS_OK;
}

I think this will update XPTI every time plugins are asked to load AND changes
are detected, so this will be done at start up (when Java wants it) or when we
first get to a page with a plugin, but only given that plugins changed somehow.

Comment 7

[Peter Lubczynski]

+  if (NS_FAILED(rv))
+    return rv;

But that won't refresh XPTI if plugins have not changed. One plugin would have
to be 'touched' in order to trigger this scan.

I think it better we scan XPTI anytime we try to "find plugins", but not twice.

Comment 8

[John Bandhauer]

Peter: I'm not gonna push real hard here. I do have some thoughts...

I question whether the scenario you suggest is common enough to justify the
*potential* startup slowdown for all users. I think you are trying to deal with
the case where users manually copy in plugins without copying in the xpt files
and then later copy in the xpt files after discovering that the plugin does not
work. [Am I missing your point?] I agree that some people might do that. I also
think that this is more common in the current situation for mozilla builds
because people are trying to manually leverage the plugins shipped with NS
builds. I really doubt that this unsupported activity needs to be automatically
handled if the cost is at all significant for everyone on every startup.

Someone is going to have to *test* the impact on startup time to decide if the
benefit to a few is worth the cost to all.

If you retain the bit of code that does an unconditional xpti autoreg for
navigator.plugins.refresh() then we can add a recomendation in the release notes
to type that string into the url bar for those few people who get stuck. [Note,
don't you endup possibly doing this call twice? Once with the call you added and
once because xpcom is being told to autoreg in the case where plugins changed?]

FWIW, the xpti autoreg call causes the following:

 - read and parse of xpti.dat
 - scan all files in components dir and the various plugin dirs.
 - build a list of those files whose names end in .xpt, .zip, or .jar
 - in the normal 'nothing changed' case, the length of that list will match 
   the length of the list of files in xpti.dat
 - for each file in the list get its timestamp and size and verify it matches the
   info from xpti.dat.

So, the minimum notable impact is the i/o to open and read one file, to scan n
directories, and to stat m files to get timestamp and size info.

You might want to verify how many times this all *really* happens

if you add to your environment something like...

set MOZILLA_XPTI_REGLOG=c:\temp\xpti\log

...before running you will get a log of xpti autoreg calls and how much work was
done. This works in release build too.

Comment 9

[John Bandhauer]

ahem. I meant:

set MOZILLA_XPTI_REGLOG=c:\temp\xpti.log

Really this can be the full path of any filename.

Comment 10

[av (gone)]

John, I discussed this with Peter having similar thoughts. I think we agreed
that trying to fix the situation when somebody drops a plugin and then suddenly
realizes that xpt file is needed too, is a bit of overkill. Such a person will
indeed be smart enough to try typing navigator.plugins.refresh() in the url bar.
The simplified patch is coming. BTW, thanks for itemizing what xpti reg does.

Comment 11

[Peter Lubczynski]

Attachment: patch v.2

This patch will only refresh XPTI if we have detected that there has been a
change to a timestamp of a plugin or plugins were added/removed. I used the
nifty environment variable to verify--we don't scan XPTI at startup unless one
plugin DLL was 'touched' or installed. (which doesn't happen very often). This
is also part of the "dynamic scanning" done before we show the default plugin. 


One drawback is that it does do the XPTI scan twice (second time should be just
stats) if plugins have changed and a plugins.refresh() is done: once in the
component manager's autoregster and once now here in plugin code. If plugins
have not changed (which is the majority of the time), it will only scan once
due to the component manager's autoregister.

Comment 12

[av (gone)]

Comment on attachment 89562 [details] [diff] [review]
patch v.2

I think this will be a reasonable solution. r=av

Comment 13

[John Bandhauer]

Comment on attachment 89562 [details] [diff] [review]
patch v.2

sr=jband

Comment 14

[Peter Lubczynski]

patch in trunk, marking FIXED

Comment 15

[shrirang khanzode]

i have verified the fix on trunk 0702. looks/works nicely now.

Comment 16

[shrirang khanzode]

sweet bugzilla wants me to add a comment. Here :blahhh

Comment 17

[Peter Lubczynski]

nominating

Comment 18

[chris hofmann]

Comment on attachment 89562 [details] [diff] [review]
patch v.2

a=chofmann for 1.0.1 add the fixed1.0.1 keyword after checking in.

Comment 19

[Peter Lubczynski]

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

Comment 20

[Peter Lubczynski]

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

Comment 21

[Paul Wyskoczka]

Adding adt1.0.1+ on behalf of adt.  Please check into the Mozilla branch asap.

Comment 22

[Peter Lubczynski]

patch in branch

Comment 23

[shrirang khanzode]

verified fixd on brnch 0723