Code in nsObjectFrame.cpp attempts to query the plugin for special variables "nsPluginInstanceVariable_WindowlessBool" and "nsPluginInstanceVariable_TransparentBool". It queries these values without pre-initializing the result value. No plug-ins provide default values for unknown variable values. Thus, for plugins that do not understand these values (the vast majority, including all sample plugins), the value remains uninitialized. This code still works in the vast majority of cases, as any value other than exactly PR_TRUE will be rejected, and the random uninitialized value is unlikely to be exactly PR_TRUE. However, it *will* happen at some stage, causing plugins to be considered "windowless", and therefore will not be displayed. Patch that initializes these values follows.

Nudging this bug - it seems quite trivial, and once every blue moon may cause bad things to happen to pages with plugins.

Old patch still applied, but with 900 line offset using fuzz :)

Instead of providing the default values, would not it be better to check whether mInstance->GetValue returned an error? That would seem safer...

Not sure I agree 100% that NS_SUCCESS will always imply a correctly initialized value. But here is a patch anyway. I have chosen not to obsolete the old one incase a reviewer happens to agree with me ;-)

Actually, this is not what I meant. What I meant is - if GetValue fails, is it wise to just fall back to a defaultvalue and continue? Wouldn't it be safer to just abort (e.g. immediatelly return whatever GetValue returned)?

No - we can not abort in this case. All GetValue() implementations only do something sensible for values they directly support. Eg, if you look in the sample plugin source code, you will see that NS_ERROR_NOT_IMPLEMENTED or NS_ERROR_FAILURE are common values. NS_SUCCESS is only returned when the specific variable name is directly supported - and in the sample plugins these variables never are. Therefore, failure will be returned for *all* of the plugin samples, and almost all plugins that exist. With the code in question, it is obvious to me that aborting out is *not* an option. These plugins must allow processing to continue with the default value of FALSE (ie, not transparent and not windowless). Insisting that all plugins support these variables is also not an option for b/w compatability.

There is one other place where the variable is uninitialised on line 1672. I would add it into the patch but my build is broken at the moment.

Updated the patch to include the extra variable caught by David. Reverted back to the original patch style - don't check the GetValue() return code - just init the value before making the call.

Comment on attachment 70235 [details] [diff] [review] Updated patch that initializes the 3 identified uninitialized variables Way to go, r=av

Comment on attachment 70235 [details] [diff] [review] Updated patch that initializes the 3 identified uninitialized variables These technically aren't unitialized variables, since they are out parameters, so the initialization is pointless. sr=beard

Comment on attachment 70235 [details] [diff] [review] Updated patch that initializes the 3 identified uninitialized variables a=scc

> These technically aren't unitialized variables, since they are > out parameters, so the initialization is pointless I'm not sure. I am fairly certain I discovered this in the debugger rather than by examination - ie, the uninitialized value *is* a problem. Note that most call sites are C++ implemented, and AFAIK, there is no magic involved that will cause these out params to be automatically intialized. But whatever :) av - you OK to check this in?

Checked in. Thanks all for taking care of the issue.

verif patch was checked in.