359873 - store the os arch in pluginreg.dat, make sPluginHostImpl::ReadPluginInfo() return failure if OS arch doesn't match, bump plugin registry version from 0.08 to 0.09

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect)

Description

[(not reading, please use [e-mail] instead)]

store the os arch in pluginreg.dat, make sPluginHostImpl::ReadPluginInfo() return failure if OS arch doesn't match, bump plugin registry version from 0.08 to 0.09

to fix bug #356694 ("Flash doesn't work on Intel Macs, problems with "Flash Player Enabler.plugin" (which is PowerPC)"), we are going to the second part of sayrer's fix for bug #346954 ("feed MIME types trigger plugin reloads")

josh and I were chatting and this is another approach.

bumping the version will cause nsPluginHostImpl::ReadPluginInfo() to fail (the first time), and if the stored archictecture of the pluginreg.dat doesn't match current OS arch, we'd fail as well.

if we do that, we could check back in sayrer's second part of the fix for bug #346954, because once the os arch and plugin version match, we can rely on the plugin cache.

http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.cpp#205

comments?

Comment 1

[Robert Sayre]

(In reply to comment #0)
> 
> comments?
> 

If josh and jst are ok with the risk, I would prefer this.

Comment 2

[Josh Aas]

I'd rather not do this on the 1.8 branch. That patch we're backing out doesn't do anything terribly important and its perfectly safe.

Comment 3

[Robert Sayre]

This resulted in a totally unacceptable perf hit in 2.0.0.1. 

Backing out part of bug #346954 seems like the culprit.

Comment 4

[Josh Aas]

(In reply to comment #3)
> This resulted in a totally unacceptable perf hit in 2.0.0.1. 
> 
> Backing out part of bug #346954 seems like the culprit.

We've had the original behavior since long before the patch that was initally landed, so I don't see how it could be an "unacceptable" perf hit. Gaining performance by doing something incorrectly should not up the bar for perf requirements.

Comment 5

[Robert Sayre]

(In reply to comment #4)
> 
> We've had the original behavior since long before the patch that was initally
> landed, so I don't see how it could be an "unacceptable" perf hit.

Simple. We load plugin libs every time we visit a feed. We didn't used to have feeds.

> Gaining performance by doing something incorrectly 

Gaining anything by doing something incorrectly is bad. The pluginreg.dat format is flawed, just like all of the legacy "keep it simple" file formats we have.

> should not up the bar for perf requirements.

I am not suggesting that we have to make trade off. We should fix the edge case of PPC-to-Intel profile imports and not sniff the same libraries over and over, every time we visit a feed.

Comment 6

[Robert Sayre]

(In reply to comment #5)
> 
> We should fix the edge case
> of PPC-to-Intel profile imports and not sniff the same libraries over and over,
> every time we visit a feed.

After making sure that the library sniffing is the cause of the perf hit, of course. It is a likely suspect.

Comment 7

[Jay Patel [:jay]]

Let's figure out the right thing to do that is safe for the 1.8 branch (just fix the edge case?).  Assigning to Josh for further investigation.

Comment 8

[Robert Sayre]

Josh, 

Are you going to have time for this in the foreseeable future? If not, I will take it.

Comment 9

[Robert Sayre]

taking per IRC discussion w/ josh

Comment 10

[Benjamin Smedberg]

We do this.