Closed Bug 701875 Opened 12 years ago Closed 12 years ago

Rename omni.jar to omni.ja

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(firefox8?, firefox10 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla11
Tracking Flags:
Tracking Status
firefox8 ? ---
firefox10 --- verified

People

(Reporter: mwu, Assigned: mwu)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, Whiteboard: [qa+], [qa!:10])

Attachments

(1 file, 1 obsolete file)

Rename omni.jar to omni.ja
4.65 KB, patch
ted
: review+
akeybl
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta-
akeybl
: approval-mozilla-release-
Details | Diff | Splinter Review 
Files that end with .jar are not backed up by system restore, causing crashes like bug 691847.
Attached patch Rename omnijar on windows (obsolete) — — Splinter Review 
This patch is running on try and results will be posted here.
Attachment #573931 - Flags: review?(khuey)
tracking-firefox8: --- → ?
There's a lot of other extensions that would work:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa378870%28v=vs.85%29.aspx

Can we pick something that's not DLL?

Two silly possibilities from that list:
    JA
    JBR
Plus, if we pick something that's not .dll, we could just use it on all platforms without being totally silly.
Try run for 147cc6fff826 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=147cc6fff826
Results (out of 47 total builds):
    success: 45
    warnings: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mwu@mozilla.com-147cc6fff826
Comment on attachment 573931 [details] [diff] [review]
Rename omnijar on windows

Sure I can do .ja

Want to take this review?
I suppose having different extension across platforms may be problematic for support and documentation. Apart .ja, there is also .ar that sounds more like "archive".
It will be desirable changing extension on all platforms because omni.jar is not a valid zip archive according to a 7-Zip developer.
http://sourceforge.net/projects/sevenzip/forums/forum/45797/topic/4014723
I'll r+ changing the extension to whatever, but we should choose something that's consistent across platforms and not totally non-nonsensical.

Note that whatever we choose, we need to verify on Windows XP.  The list from comment 2 is for Vista and later.
both .ja and .ar are in the XP list, that is stored in system32/Restore/filelist.xml, this file doesn't exist anymore in Vista or newer.
I don't think I can attach that file here though.
why not the extension ".omni" ?
Since they called the file in this new format omni.jar

Btw, if one
1) Unpacks the omni.jar file
2) Makes minor changes to the contents
3) Repacks the contents (as a standard zip file)
4) Renames to omni.jar

Then according to my tests, Firefox can't load it, and won't start.

However if step 2 is omitted (i.e. the content is unchanged), then FF has no problem reading it.

Also the loading time gains can be gained if one recompresses the standard .jar files in previous version.   Recompressing reduces .jar files to about 1/3 the size.

Since FF is unable to read standard .jar (.zip) files, then obviously they can't honestly pretend to be using the .zip format.
(In reply to andré from comment #10)
> why not the extension ".omni" ?
> Since they called the file in this new format omni.jar

Because .omni won't get backed up by System Restore, and won't solve the problem this bug was filed for ...
(In reply to Marco Bonardo [:mak] from comment #9)
> both .ja and .ar are in the XP list, that is stored in
> system32/Restore/filelist.xml, this file doesn't exist anymore in Vista or
> newer.
> I don't think I can attach that file here though.

Excellent, that's what we needed to know.
(In reply to Marco Bonardo [:mak] from comment #6)
> I suppose having different extension across platforms may be problematic for
> support and documentation. Apart .ja, there is also .ar that sounds more
> like "archive".

Based on a web search, .ja is an "IBM Tools Updater file" (which seems to not be used any more), .ar is a *nix archive format - the predecessor to .tar, still used for static libraries. Using .ar for something that isn't an ar archive seems more odd to me than using .ja.
Why Microsoft reserved .ar for System Restore?
JA, KO, UK, US, ZH, ... look to be country codes. Doesn't AR mean "Argentina" here?
Comment on attachment 573931 [details] [diff] [review]
Rename omnijar on windows

r- per comments.
Attachment #573931 - Flags: review?(khuey) → review-
This bug report is trying to solve a bug in Ms-windows, not Mozilla.
Also note that if system restore doesn't back up .jar files, it wouldn't have been doing so in pre-omni format Mozilla, so nothing has changed.
It doesn't matter whose bug it is. It's affecting our users in a negative manner, so we need to do something about it.
mwu: I'll r+ a patch to use .ja. It's silly, but if it works that's all that matters. Can you spin a try build with that so QA can test that that works with system restore?
Would this bug preclude the need for 701944 once fixed?
I pushed this patch to try.
Attachment #573931 - Attachment is obsolete: true
Attachment #574066 - Flags: review?(ted.mielczarek)
Summary: Rename omni.jar to omnijar.dll on Windows → Rename omni.jar to omni.ja
OS: Windows XP → All
(In reply to Alex Keybl [:akeybl] from comment #19)
> Would this bug preclude the need for 701944 once fixed?

Yes, most likely. I don't expect the warning to show once we have this landed. However, being paranoid may be worth it.
I am slightly worried that we are just fighting the most prominent case and the one we're seeing now but other files will still not be covered and cause similar problems - have we checked for that?
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #22)
> I am slightly worried that we are just fighting the most prominent case and
> the one we're seeing now but other files will still not be covered and cause
> similar problems - have we checked for that?

I have examined other files and the only one that might pose a problem is the install.rdf for the default theme. The dictionary and search plugin files are also exposed, but I don't expect them to cause problems. (though it is probably worth pulling them into the omnijar to keep them safe)
Comment on attachment 574066 [details] [diff] [review]
Rename omni.jar to omni.ja

Review of attachment 574066 [details] [diff] [review]:
-----------------------------------------------------------------

Good call on making it a configure variable. r=me assuming the try build passes, and QA can test that it fixes the system restore issue.
Attachment #574066 - Flags: review?(ted.mielczarek) → review+
Try run for 3a43447ada42 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=3a43447ada42
Results (out of 47 total builds):
    success: 44
    warnings: 3
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mwu@mozilla.com-3a43447ada42
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #22)
> I am slightly worried that we are just fighting the most prominent case and
> the one we're seeing now but other files will still not be covered and cause
> similar problems - have we checked for that?

eg the .chk files which NSS uses are not included by System Restore, but the dll's they pair with are. IIRC having mismatched files breaks FIPS mode, which is the US government standard for cryptography. But short of opting out of System Restore entirely I can't see how we can avoid such situations.
I've tested this try-server on WinXP build by:

* installing this try server build first
* creating a restore point
* installing a copy of the latest nightly (pave over installation)
* restoring back the checkpoint and launching

I've used this procedure to reproduce the problem with other builds like 7.0.1 and 8.0.

Here, the installation folder after the restore has an omni.jar and omni.ja file. The application launches and I see no obvious problem.
mwu - can you elaborate on your comment in bug 701944 where you say you don't expect any regressions from this bug (specifically in RelEng or add-on compatibility)? I just want to make sure we're considering all the places omni.jar may be used/hardcoded. Thanks!
Did we check that repacks still work? As they kinda need to do a dance to unpack and repack the omnijar.
(In reply to juan becerra [:juanb] from comment #27)
> I've tested this try-server on WinXP build by:
> 
> * installing this try server build first
> * creating a restore point
> * installing a copy of the latest nightly (pave over installation)
> * restoring back the checkpoint and launching
> 

omni.jar should be copied over omni.ja after installing the latest nightly to make sure that system restore restores the original omni.ja.
(In reply to Axel Hecht [:Pike] from comment #29)
> Did we check that repacks still work? As they kinda need to do a dance to
> unpack and repack the omnijar.

The packing/unpacking code is all centralized in packager.mk and all the hard coding there has been removed. I have also successfully repacked a linux en-US build to linux ja.

(In reply to Alex Keybl [:akeybl] from comment #28)
> mwu - can you elaborate on your comment in bug 701944 where you say you
> don't expect any regressions from this bug (specifically in RelEng or add-on
> compatibility)? I just want to make sure we're considering all the places
> omni.jar may be used/hardcoded. Thanks!

omni.jar is hard coded in two places:
1. Packaging code (packager.mk) - this has been tested in the two important cases - initial packaging and locale repacking, and tends to cause very visible breakage when things go wrong.
2. The main omnijar code (Omnijar.cpp). Everything else uses the omnijar API to figure out where the omnijar is, and even then, that's only for very low level initialization code, not extensions. Extensions are generally are not aware of what an omnijar is - it's just a very very low level implementation detail which is buried by resource and chrome URIs. Even our frontend code has no concept of what an omnijar is. Basically, the right thing is so much easier to do than the wrong thing here that it shouldn't be an issue.
(In reply to Michael Wu [:mwu] from comment #30)
> (In reply to juan becerra [:juanb] from comment #27)
> > I've tested this try-server on WinXP build by:
> > 
> > * installing this try server build first
> > * creating a restore point
> > * installing a copy of the latest nightly (pave over installation)
> > * restoring back the checkpoint and launching
> > 
> 
> omni.jar should be copied over omni.ja after installing the latest nightly
> to make sure that system restore restores the original omni.ja.

Just deleting omni.ja would be simpler.
I tested on vista by deleting the omnijar and ensuring system restore restored the file.

https://hg.mozilla.org/mozilla-central/rev/3f0b94325b80
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Attachment #574066 - Flags: approval-mozilla-release?
Attachment #574066 - Flags: approval-mozilla-beta?
Attachment #574066 - Flags: approval-mozilla-aurora?
Did we chech if we can convince Windows to put omni.jar in system restore with its original neame before we went diwn this avenue? Curtis says this this should probably be possible from what he knows from his days working there.
'make package' started to fail for Thunderbird on Linux with

+++ Failed to get ScriptSecurityManager service, running without principalsSegmentation fault
make[2]: *** [make-package] Fehler 139

Tinderbox is unhappy too: <http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTrunk/1321157115.1321161379.14187.gz>

Local backout of the patch <http://hg.mozilla.org/mozilla-central/rev/3f0b94325b80> fixes the bustage.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #34)
> Did we chech if we can convince Windows to put omni.jar in system restore
> with its original neame before we went diwn this avenue? Curtis says this
> this should probably be possible from what he knows from his days working
> there.

We should consider this, while keeping in mind that this would not resolve 8.0.1 restored down to 7, only 9 restored down to 8.0.1.
What do you mean by "convince"?  There's no API to do that.  If you mean convincing Microsoft to patch Windows to do that, it seems like that would take a lot longer than fixing it on our end.
Follow up to fix non-omnijar builds on Windows by always defining OMNIJAR_NAME:
https://hg.mozilla.org/mozilla-central/rev/7ae3089be8a5
I ported the patch to comm-central and landed it.
https://hg.mozilla.org/comm-central/rev/259ba99bda29
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #37)
> What do you mean by "convince"?  There's no API to do that.

Curtis searched a bit and said he found something that looks like registry settings that might provide that API.
Unless those are documented on MSDN I don't think we should go there.
per chemspill meeting: this fix is to help users who upgrade, then downgrade (ie 7.0.1 -> 8.0.1 -> 7.0.1). 

At this time, the consensus was that there's too much risk taking this for the FF8.0.1 chemspill. Once we have better scope/risk understanding of this change, we should revisit for a possible non-chemspill 8.0.2 or possibly wait until 9.0.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #41)
> Unless those are documented on MSDN I don't think we should go there.

IIRC, is was on MSDN where he found this setting for the exclusion list of system restore, but he would know more details. Curtis?
Would system restore work if the file didn't have an extension at all? (which would be better, imho, than abusing an unrelated file type)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #43)
> IIRC, is was on MSDN where he found this setting for the exclusion list of
> system restore, but he would know more details. Curtis?

I tried to look around, but I only found APIs to store a SR point or restore to a SR point. Btw, system restore changed across XP vs Vista/7 (in xp the list is controlled by an xml, that has been removed later), thus I'd expect that even if you could convince them to support jar files, I doubt they'd patch XP.
The maximum I found in the registry is HKEY_Local_Machine\System\CurrentControlSet\Control\BackupRestore\FilesNotTobackup that is a supported multistring but doesn't really help our case.
http://msdn.microsoft.com/en-us/library/windows/desktop/bb891959%28v=vs.85%29.aspx
Comment on attachment 574066 [details] [diff] [review]
Rename omni.jar to omni.ja

[Triage Comment]
Denying for approval since this patch does not yet fix the problem of FF(n) system restored down to FF(n-1), only FF(n+1) down to FF(n) where n is the version that this patch first lands in.

We also need to make sure this is still the right thing to do for FF9 and up. Please sync up with Rob Strong, who's taking lead on the system restore issue for FF9.
Attachment #574066 - Flags: approval-mozilla-release?
Attachment #574066 - Flags: approval-mozilla-release-
Attachment #574066 - Flags: approval-mozilla-beta?
Attachment #574066 - Flags: approval-mozilla-beta-
Attachment #574066 - Flags: approval-mozilla-aurora?
Attachment #574066 - Flags: approval-mozilla-aurora-
(In reply to Alex Keybl [:akeybl] from comment #46)
> [Triage Comment]
> Denying for approval since this patch does not yet fix the problem of FF(n)
> system restored down to FF(n-1), only FF(n+1) down to FF(n) where n is the
> version that this patch first lands in.
> 

Actually, this can fix FF(n-1) if omni.jar isn't removed.

> We also need to make sure this is still the right thing to do for FF9 and
> up. Please sync up with Rob Strong, who's taking lead on the system restore
> issue for FF9.

This works.
Target Milestone: --- → mozilla11
Filed Bug 707254 to not delete the omni.jar
Comment on attachment 574066 [details] [diff] [review]
Rename omni.jar to omni.ja

The decision to deny for approval-mozilla-aurora and approval-mozilla-beta was originally made because at the time this was considered risky, and if it didn't fix our FF9->FF8 story (without bug 707254), it wasn't considered worth it.

Rob is making the case for now taking this on FF9 Beta to fix FF10->FF9, thus preventing the need for 707254 in FF10. He feels the fix is low risk and understood, considering the bake time it's already recieved on m-c.

We'll be discussing at today's 2PM PT channel meeting.
Attachment #574066 - Flags: approval-mozilla-beta?
Attachment #574066 - Flags: approval-mozilla-beta-
Attachment #574066 - Flags: approval-mozilla-aurora?
Attachment #574066 - Flags: approval-mozilla-aurora-
(In reply to Alex Keybl [:akeybl] from comment #49)
> We'll be discussing at today's 2PM PT channel meeting.

Dave/Jorge - can you speak to the add-on compatibility risk of uplifting the omni.jar rename from m-c to beta given its bake time? My spidey sense tells me that we'd want to allow the fix to bake on FF10 aurora/beta to be sure and not touch FF9, but I wanted to check with you all about how common reading/writing the omni.jar in extensions/plugins may be.
(In reply to Alex Keybl [:akeybl] from comment #50)
> (In reply to Alex Keybl [:akeybl] from comment #49)
> > We'll be discussing at today's 2PM PT channel meeting.
> 
> Dave/Jorge - can you speak to the add-on compatibility risk of uplifting the
> omni.jar rename from m-c to beta given its bake time? My spidey sense tells
> me that we'd want to allow the fix to bake on FF10 aurora/beta to be sure
> and not touch FF9, but I wanted to check with you all about how common
> reading/writing the omni.jar in extensions/plugins may be.

I see about 4 add-ons explicitely referencing omni.jar by name, mostly just using our images themselves, even that makes me wary of saying it'd be ok to move this to beta at this point. Kev might have knowledge of whether enterprises and others play with omni.jar and if their tools might get broken by this.
(In reply to Dave Townsend (:Mossop) from comment #51)
> I see about 4 add-ons explicitely referencing omni.jar by name, mostly just
> using our images themselves, even that makes me wary of saying it'd be ok to
> move this to beta at this point. Kev might have knowledge of whether
> enterprises and others play with omni.jar and if their tools might get
> broken by this.

The question is why they would want to do that at all, when they could just use resource:// uris. Is there any lack of related documentation? Or are people just always innovative in ways to screw things up?
(In reply to Mike Hommey [:glandium] from comment #52)
> (In reply to Dave Townsend (:Mossop) from comment #51)
> > I see about 4 add-ons explicitely referencing omni.jar by name, mostly just
> > using our images themselves, even that makes me wary of saying it'd be ok to
> > move this to beta at this point. Kev might have knowledge of whether
> > enterprises and others play with omni.jar and if their tools might get
> > broken by this.
> 
> The question is why they would want to do that at all, when they could just
> use resource:// uris. Is there any lack of related documentation? Or are
> people just always innovative in ways to screw things up?

Extension developers will find imaginative ways of getting to what they want and if there is an obvious way to do something one way you may not search docs to find an alternate. In this case they are using jar:resource://app/omni.jar! to get into it.
It should be documented anyway.
Keywords: addon-compat, dev-doc-needed
(In reply to Dave Townsend (:Mossop) from comment #53)
? In this case they are using jar:resource://app/omni.jar! to get into it.

O_o that shouldn't even work. resource://app/ points in the omni.jar already.
(meaning, jar:resource://app/omni.jar! would point to jar:jar:file:///path/to/omni.jar!/omni.jar!)
(In reply to Alex Keybl [:akeybl] from comment #50)
> (In reply to Alex Keybl [:akeybl] from comment #49)
> > We'll be discussing at today's 2PM PT channel meeting.
> 
> Dave/Jorge - can you speak to the add-on compatibility risk of uplifting the
> omni.jar rename from m-c to beta given its bake time? My spidey sense tells
> me that we'd want to allow the fix to bake on FF10 aurora/beta to be sure
> and not touch FF9, but I wanted to check with you all about how common
> reading/writing the omni.jar in extensions/plugins may be.

I think I discussed this with Christian before, and the result was similar to what Dave mentioned. There are a few add-ons that refer to the JAR file directly, but it is clearly some hacky workaround or the only/first way they made something work. IIRC, they were mostly references to images and stylesheets, so the breakage wouldn't be major.

While I agree that it is good to be cautious and cover all bases, I'm OK with moving this to beta. We can use the Add-ons Blog and the compatibility mailing list to get the message out and avoid too many surprises.
Whiteboard: [qa+]
Comment on attachment 574066 [details] [diff] [review]
Rename omni.jar to omni.ja

[Triage Comment]
After talking with rs about this during the channel meeting, we decided to take on FF10 only due to how late we are in the cycle. We'll consider taking 702045 as well if/when it's ready, which may give us the same behavior as taking this alone in FF9.
Attachment #574066 - Flags: approval-mozilla-beta?
Attachment #574066 - Flags: approval-mozilla-beta-
Attachment #574066 - Flags: approval-mozilla-aurora?
Attachment #574066 - Flags: approval-mozilla-aurora+
mwu, can you land this on Aurora? Thanks
Target Milestone: mozilla11 → mozilla10
status-firefox10: --- → fixed
Target Milestone: mozilla10 → mozilla11
(In reply to Michael Wu [:mwu] from comment #39)
> I ported the patch to comm-central and landed it.
> https://hg.mozilla.org/comm-central/rev/259ba99bda29

This needs to be transplanted to comm-aurora now, too.
I forgot to land the bustage fix too, but philor reminded me, so here it is:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5004ac327d53
Mozilla/5.0 (Windows NT 5.1; rv:10.0) Gecko/20100101 Firefox/10.0 beta 6

Verified using the following cases on Windows XP with firefox 10 beta 6 (performing pave-over install with Firefox 9.0.1 and Firefox 11 Aurora):

1. Install Firefox 10B6. 
2. Create a System restore Point. 
3. Pave-over install Firefox 9.0.1
4. Delete omni.jar.
5. restore to the system point created in step 2

And 

1. Install Firefox 10B6. 
2. Create a System restore Point. 
3. Pave-over install Firefox 11 Aurora.
4. Delete omni.jar.
5. restore to the system point created in step 2


In both cases, omni.ja was restored and Firefox was launched normally. 

I can see that the Platform field is set to all, but don't know of anything similar to Windows restore point for Mac OS and Linux. if this can be verified in any way on these platforms, please specify.
Status: RESOLVED → VERIFIED
status-firefox10: fixed → verified
Whiteboard: [qa+] → [qa+], [qa!:10]
The optimizejars.py script no longer works.

http://mxr.mozilla.org/mozilla-central/source/config/optimizejars.py
(In reply to prajatarah from comment #65)
> The optimizejars.py script no longer works.
> 
> http://mxr.mozilla.org/mozilla-central/source/config/optimizejars.py

please file a new bug for that, blocking this one.
Depends on: 726656
(In reply to Marco Bonardo [:mak] from comment #66)
> (In reply to prajatarah from comment #65)
> > The optimizejars.py script no longer works.

filed as bug 726656.
Documentation updated:

https://developer.mozilla.org/en/Mozilla/About_omni.ja_%28formerly_omni.jar%29

Mentioned on Firefox 11 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.