Closed Bug 648732 Opened 13 years ago Closed 13 years ago

Testing add-ons unpacked creates unrealistic results

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jwkbugzilla, Assigned: anodelman)

References

Details

Attachments

(1 file)

Currently Talos will unpack all extensions when installing them into the newly created Firefox profile. This is not a good approach if the goal is to get results that are close to real-world scenarios - Firefox 4 will usually only unpack extensions if <em:unpack> flag is specified. This of course mostly effects cold startup performance whereas Talos is testing warm startup. Still, my tests indicate that leaving add-ons unpacked might change measured performance by 3% of the Firefox startup time in either direction which is significant.
How are addons currently installed in user profiles?  Do we now only installed as a zip by default (other then the <em:unpack directive)?

I believe that our installation code has aged since it was first created and my no longer properly reflect how installation is done in the real world.
Mossop should be able to give a definite answer to comment #1, but AFAIK all add-ons that don't have em:unpack are just copied into @profile@/extensions/ aand renamed to @ext-id@.xpi while those with en:unpack are extracted into a @ext-id@ directory.

Would there be some way to just actually make Firefox do the install through its own internal code?
From all I know, Robert is correct.

Unfortunately, I don't know of any way to make Firefox install an extension - putting an XPI into profile used to work pre-Firefox 4 but even that required user interaction. This is probably intended, a feature like that would likely be abused by third-parties.
(In reply to comment #2)
> Mossop should be able to give a definite answer to comment #1, but AFAIK all
> add-ons that don't have em:unpack are just copied into @profile@/extensions/
> aand renamed to @ext-id@.xpi while those with en:unpack are extracted into a
> @ext-id@ directory.

This is correct.

> Would there be some way to just actually make Firefox do the install through
> its own internal code?

I guess you could write an extension to do this, might be overkill though.
If there is no means to force install a zipped addon without user intervention
then we should continue with the current installation process and accept that
there is a difference in the results - that's fine as long as the difference is
consistent across runs.

There are always trade offs with talos configurations between what we can do automatically and what best represents real world usage.

It would simply mean shifting the arbitrary split point for what addons are
considered performant.
Is it too much work to unzip the file, look for the unpack flag and then choose the installation method?

Given that unpack is set to false by default, and the reasons to set it to true are rare (less rare on top add-ons, though), I'd rather fail on the side of not unpacking add-ons.
I thought from:

"
putting an XPI into profile used to work pre-Firefox 4 but even that required
user interaction.
"

that this wasn't an option?
Sorry, you misunderstood me - using an XPI file used to trigger the "proper" installation process. Now you have to put the XPI file and name it properly yourself, then it is simply installed. You are responsible for doing everything right because the browser's installation routine will not kick in.

So in the end you need to read install.rdf from that ZIP file, check for <em:unpack>true</em:unpack> and if it is found - unpack as it is done now. If that flag isn't found - simply move the XPI file into the extensions subdir as @addon_id@.xpi.
Mossop, i actually just occurred to me that we do have access to the application install directory on Talos anyhow and that we do install add-ons from @appdir@/distribution/extensions/ into the user profile in 4.0 and later - does this mechanism do "the right thing" for packed vs. em:unpack add-ons?
Of course, this does the "install" on the first startup, but we discard perf results of that one anyhow, so that shouldn't be a concern, I guess.
(In reply to comment #9)
> Mossop, i actually just occurred to me that we do have access to the
> application install directory on Talos anyhow and that we do install add-ons
> from @appdir@/distribution/extensions/ into the user profile in 4.0 and later -
> does this mechanism do "the right thing" for packed vs. em:unpack add-ons?
> Of course, this does the "install" on the first startup, but we discard perf
> results of that one anyhow, so that shouldn't be a concern, I guess.

Sadly no, that doesn't yet do the right thing in this regard
Assignee: nobody → anodelman
Tested on tools-r3-leopard-001 with PersonasPlus and FoxClocks, PersonasPlus does not request unpacking while FoxClocks does.  Worked for each and the addons then showed up in the addon manager correctly installed.
Comment on attachment 529770 [details] [diff] [review]
[checked in]only unpack extension if em:unpack is true

An additional review won't hurt - looks good to me, this is similar to the code I have been working with myself.
Attachment #529770 - Flags: review+
Comment on attachment 529770 [details] [diff] [review]
[checked in]only unpack extension if em:unpack is true

Review of attachment 529770 [details] [diff] [review]:

Code wise this looks good.  I assume this is for addons only and not for pageloader and other core talos extensions (like mobile tpan/tzoom and electrolysis shim)?
Attachment #529770 - Flags: review?(jmaher) → review+
jmaher - yes this is for addons only.  It won't affect the installation of the pageloader or the mobile tpan/tzoom shim.
Comment on attachment 529770 [details] [diff] [review]
[checked in]only unpack extension if em:unpack is true

changeset:   235:d1afa96e783d, from bug 648978.
Attachment #529770 - Attachment description: only unpack extension if em:unpack is true → [checked in]only unpack extension if em:unpack is true
Depends on: 651670
Deployed.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 658187
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: