Always flush the JAR cache for AddonInstall temp XPIs

RESOLVED FIXED in Firefox 62



Last year
Last year


(Reporter: kmag, Assigned: aswan)


Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed)



(1 attachment)

There are weird corner cases where we sometimes attempt to re-use the temp filename from an older install attempt for a new install, and therefore wind up reading the cached data in the jar cache from a different add-on. This probably isn't possible on Windows (but may prevent us from deleting the temp files), and is probably extremely unlikely to happen in the real world. But we should probably fix it anyway. If nothing else, it would probably save some resident memory.
Is there a reason not to flush here unconditionally:

There are a few things that touch the zip reader that don't set that flag (eg findEntries() ), I'm not sure exactly what does and doesn't require a flush.  But the bigger issue is that we bypass the XPIPackage instance and just use a jar url here:

I guess we could manually set needFlush on the package at that line, that feels clumsy though.  Is there some scenario where an unconditional flush would add too much unnecessary overhead?
Flags: needinfo?(kmaglione+bmo)
We should probably just flush unconditionally, yeah. I added the `needFlush` property because I was trying to preserve the old behavior when I added a new fetch call that wound up putting the XPI in the jar cache. But for WebExtensions, where we already loaded the manifest from jar: channels, this problem already existed.
Flags: needinfo?(kmaglione+bmo)
Attachment #8974537 - Flags: review?(kmaglione+bmo)
Comment on attachment 8974537 [details]
Bug 1460095 Always flush the jar cache after looking at an xpi file

Attachment #8974537 - Flags: review?(kmaglione+bmo) → review+
Pushed by
Always flush the jar cache after looking at an xpi file r=kmag
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee: nobody → aswan
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(aswan)
Flags: needinfo?(aswan) → qe-verify-
You need to log in before you can comment on or make changes to this bug.