Closed Bug 1070988 Opened 5 years ago Closed 5 years ago

Installer should remove leftover chrome.manifest (on Windows) Startup (crash at pave-over upgrade of Firefox 31 with unpacked omni.ja)

Categories

(Firefox :: Installer, defect, major)

32 Branch
x86_64
Windows 7
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 35
Tracking Status
firefox32 --- wontfix
firefox33 + verified
firefox34 + verified
firefox35 --- verified
firefox-esr31 --- verified

People

(Reporter: FlorinMezei, Assigned: rstrong)

References

Details

Crash Data

Attachments

(1 file, 1 obsolete file)

Reproducible with:
Firefox 32.0.2 - BuildID: 20140917194002
Firefox 33 Beta 5 - BuildID: 20140918174809
Latest Firefox 34 Aurora - BuildID: 20140922004004
Latest Firefox 35 Nightly - BuildID: 20140922030204

Environment: Windows 7 x64

Steps to reproduce:
1. Install Firefox 31 cleanly in a new empty folder.
2. In the installation folder rename omni.ja to omni.zip.
3. Using Windows Explorer extract all contents of omni.zip to the installation folder.
4. Rename omni.zip back to omni.ja.
5. Download one of the affected builds (32.0.2, 33, 34, 35) and choose to install it at the same location where 31 is, thus performing a pave-over upgrade.
6. When upgrade completes, choose to launch Firefox.

Expected results: 
Firefox starts correctly without any issues.

Actual results:
Firefox crashes on startup with two different signature:
a) 32.0.2 & 33 Beta 5 - [@ nsFrame::BoxReflow(nsBoxLayoutState&, nsPresContext*, nsHTMLReflowMetrics&, nsRenderingContext*, int, int, int, int, bool)] (same as in bug 1063052)
b) 34 & 35 - [@ mozalloc_abort(char const* const) | NS_DebugBreak | ErrorLoadingBuiltinSheet ] - see bp-25124030-ba65-4bf1-9829-086a42140922 (https://crash-stats.mozilla.com/report/index/bp-25124030-ba65-4bf1-9829-086a42140922)

Notes:
- this issue is a follow up on bug 1063052 (startup crash in installations with unpacked omni.ja). Prior to this fix all branches were crashing with nsFrame::BoxReflow.
As noted in bug 1063052, there is no crash when updating through About -> Help to 32.0.2 or 33 Beta 5 (could not verify for Nightly or Aurora as update failed).
So this is basically the same issue as in bug 1063052, where we're picking up the bogus unpacked files instead of the files in the omni.ja, right?

This bug is presumably about having the installer remove the files as suggested in bug 1063052 comment 91?
Component: Layout → Installer
Flags: needinfo?(robert.strong.bugs)
Product: Core → Firefox
I think it would be a good thing to do this for the installer but I thought this bug was actually about bug 1063052 comment 81 and specifically,

"It seems to me the main part of the failure in this bug comes from the fact that chrome manifests outside the omni.ja are treated after the chrome manifests inside the omni.ja. What about doing the opposite? If something is registered from an extracted omni.ja, it will be overridden by the contents of omni.ja. If people really want to use the contents of the extracted omni.ja, they'll have to remove omni.ja."
Flags: needinfo?(robert.strong.bugs)
^^
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
So, this bug tracks reversing the priority, then? (making us defer to omni.ja instead of to the unpacked files)
We need a bug for each so might as well file a new bug for that.
"a new bug" = this bug [separate from bug 1063052], yes?
I can use this bug for the installer changes since it is already in that component and there should be a new bug for making us defer to omni.ja instead of to the unpacked files.
Summary: Startup crash at pave-over upgrade of Firefox 31 with unpacked omni.ja → Installer should remove leftover chrome.manifest (on Windows) Startup (crash at pave-over upgrade of Firefox 31 with unpacked omni.ja)
Assignee: nobody → robert.strong.bugs
Status: NEW → ASSIGNED
Attached patch patch rev2Splinter Review
fixed a typo
Attachment #8493598 - Attachment is obsolete: true
Attachment #8493598 - Flags: review?(netzen)
Attachment #8493602 - Flags: review?(netzen)
Comment on attachment 8493602 [details] [diff] [review]
patch rev2

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

Discussed in IRC
Attachment #8493602 - Flags: review?(netzen) → review+
Comment on attachment 8493602 [details] [diff] [review]
patch rev2

Approval Request Comment
[Feature/regressing bug #]: bug 1063052
[User impact if declined]: pave over installs will crash per bug 1063052
[Describe test coverage new/current, TBPL]: tested locally
[Risks and why]: extremely small
[String/UUID change made/needed]: none
Attachment #8493602 - Flags: approval-mozilla-beta?
Attachment #8493602 - Flags: approval-mozilla-aurora?
Attachment #8493602 - Flags: approval-mozilla-beta?
Attachment #8493602 - Flags: approval-mozilla-beta+
Attachment #8493602 - Flags: approval-mozilla-aurora?
Attachment #8493602 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/532ad1c88cfa
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Flags: qe-verify+
QA Contact: florin.mezei
Verified this issue with steps from comment 0, on Windows 7 x64, with:
- Firefox 33 Beta 8 - BuildID: 20140929180120
- Latest Firefox 34 Aurora - BuildID: 20140930004006.
- Latest Firefox 35 Nightly - BuildID: 20140929030205.

The application started correctly without crashing after the pave-over upgrade.
Ryan

Can you land this in ESR31 so we can test to see if this addresses the pave-over upgrade issues that are left in Bug #1055766
Flags: needinfo?(ryanvm)
When uninstalling fx, the original omni.ja files that were exported into "C:\Program Files (x86)\Nightly" are not being removed and thus leaving behind the "Nightly" folder when uninstalling. I installed the latest fx-esr31 over these files and made sure there wasn't any crashes when opening fx after installation. Other than that, everything is working correctly. The test cases can be found in bug # 1055766 comment # 57.

Robert, should I create a new bug for the above problem? Or is this something we can't fix?
Flags: needinfo?(robert.strong.bugs)
No bug needs to be filed... it isn't something we want to fix since these are files that the we didn't install and the user might have installed. Files that weren't installed by the installer which includes the files that were extracted from within the omni.ja are intentionally not removed.
Flags: needinfo?(robert.strong.bugs)
Thanks Robert! Figured that was the case but wanted to double check. As per comment # 19, test cases can be found in bug # 1055766 comment # 57. (Used Win 8.1 x64)
You need to log in before you can comment on or make changes to this bug.