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

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 1

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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).

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Robert Strong (they/them - no direct email)]

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."

Comment 4

[Robert Strong (they/them - no direct email)]

^^

Comment 5

[Daniel Holbert [:dholbert]]

So, this bug tracks reversing the priority, then? (making us defer to omni.ja instead of to the unpacked files)

Comment 6

[Robert Strong (they/them - no direct email)]

We need a bug for each so might as well file a new bug for that.

Comment 7

[Daniel Holbert [:dholbert]]

"a new bug" = this bug [separate from bug 1063052], yes?

Comment 8

[Robert Strong (they/them - no direct email)]

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.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 10

[Robert Strong (they/them - no direct email)]

Attachment: patch rev2

fixed a typo

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 8493602 [details] [diff] [review]
patch rev2

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

Discussed in IRC

Comment 12

[Robert Strong (they/them - no direct email)]

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

Comment 13

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/532ad1c88cfa

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/790e8ba5cc89
https://hg.mozilla.org/releases/mozilla-beta/rev/9286fb781568

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/532ad1c88cfa

Comment 16

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 17

[Benjamin Kerensa [:bkerensa]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/4ef68b98588f

Comment 19

[Kamil Jozwiak [:kjozwiak]]

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?

Comment 20

[Robert Strong (they/them - no direct email)]

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.

Comment 21

[Kamil Jozwiak [:kjozwiak]]

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)