Installing crashme-new.xpi fails to actually install

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: ted, Assigned: wesj)

Tracking

Trunk
ARM
Android

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
STR:
1) Visit the URL in the URL field
2) Click the "crashme-new" link
3) Click "Allow"
4) Click "Install Crash Me Now Advanced!"
5) Notification appears to restart, click restart in the Add-Ons pane

Expected:
"Crash Me Now Advanced!" in the Add-Ons list

Actual:
crashme did not get installed.

I don't see any telling errors in the error console, it all looks like it's going smoothly until it fails to install.

This is a self-built Android Fennec on a Nexus One built on mozilla-central changeset 8b43f0a5ae9b, and mobile changeset 7f6cee27564.
(Reporter)

Comment 1

8 years ago
I tried today's official nightly, still didn't work.
(Assignee)

Comment 2

8 years ago
I can not reproduce this with nightlies or with my own builds on my Droid X. Maybe its a problem with the Nexus One. Can you try again with extensions.logging.enabled=true and tell me what appears?
(Reporter)

Comment 3

8 years ago
Aha! After restarting:
LOG addons.xpi: Processing install of crashme-new.xpi
Error: ERROR addons.xpi: Failure moving /data/data/org.mozilla.fennec/mozilla/xnxwnx50.default/extensions/staged/crashme@ted.mielczarek.org to /data/data/org.mozilla.fennec/mozilla/xnxwnx50.default/extensions
XPIProvider.jsm Line: 221

Error: ERROR addons.xpi: Failed to install staged add-on crashme@ted.mielczarek.org in app-profile: [Exception... "Component returned failure code: ... NS_ERROR_FILE_TARGET_DOES_NOT_EXIST [nsIFile.isDirectory]" location: XPIProvider.jsm :: anonymous :: line 200" ]
XPIProvider.jsm Line: 200
(Reporter)

Comment 4

8 years ago
Note that crashme-new is an unpacked extension.

Updated

8 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
(Assignee)

Updated

8 years ago
Assignee: nobody → wjohnston
(Assignee)

Comment 5

8 years ago
Created attachment 497906 [details] [diff] [review]
Initial Fix

For some reason the nsIDirectoryEnumerator sometimes gives us an item that doesn't exist. Not sure if the problem is higher up in our code or with Android. This fixes this (at least for crashme), but I'm going to look higher up the chain before I put it up for review.
(Assignee)

Comment 6

8 years ago
Created attachment 497909 [details] [diff] [review]
Real Initial fix

Whoops. Fix an obvious error I made moving from a messy patch to a clean one.
Attachment #497906 - Attachment is obsolete: true
Attachment #497909 - Flags: review?(dtownsend)
Comment on attachment 497909 [details] [diff] [review]
Real Initial fix

I'm concerned that we don't properly understand the cause of this bug. My best guess is that on android nsIDirectoryEnumerator is a live view of the filesystem when elsewhere it seems to be static from the time of creation. If that is so then this patch would probably make us miss installing files. Wes was going to verify whether that was the case or not so I'm not going to review this till I've heard back.

Assuming that is the case then we need to enumerate the entries into an array or something before mutating the directory.
Attachment #497909 - Flags: review?(dtownsend)
(Assignee)

Comment 8

8 years ago
Created attachment 500928 [details] [diff] [review]
Cache directory entries

Based on what I've been able to read, its not smart to delete files while using readdir. So I think we are probably better off caching the list like you suggested.
Attachment #497909 - Attachment is obsolete: true
Attachment #500928 - Flags: review?(dtownsend)
(Assignee)

Comment 9

8 years ago
Comment on attachment 500928 [details] [diff] [review]
Cache directory entries

This worked to install crashme, but I'm seeing some other errors now. Removing review request while I look into them...
Attachment #500928 - Flags: review?(dtownsend)
(Assignee)

Comment 10

8 years ago
Created attachment 500951 [details] [diff] [review]
Cache v2

Whoops. Just a dumb mistake. Fixed. This also fixes bug 617523 on this Nexus One.
Attachment #500928 - Attachment is obsolete: true
Attachment #500951 - Flags: superreview?(dtownsend)
(Assignee)

Updated

8 years ago
Attachment #500951 - Flags: superreview?(dtownsend) → review?(dtownsend)
Comment on attachment 500951 [details] [diff] [review]
Cache v2

Use cacheEntries.forEach(function(aEntry) { ... }, this) instead of the for-loop.
Attachment #500951 - Flags: review?(dtownsend) → review-
(Assignee)

Comment 12

8 years ago
Created attachment 501029 [details] [diff] [review]
Patch v3

Updated to use Array.forEach.
Attachment #500951 - Attachment is obsolete: true
Attachment #501029 - Flags: review?(dtownsend)
Comment on attachment 501029 [details] [diff] [review]
Patch v3

Looks good
Attachment #501029 - Flags: review?(dtownsend) → review+
pushed:
http://hg.mozilla.org/mozilla-central/rev/267854eaa0ff
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 15

8 years ago
Verified as fixed on Mozilla /5.0 (Android;Linux armv7l;rv:2.0b9pre) Gecko/20100107 Firefox/4.0b9pre Fennec /4.0b4pre 
with : Motorola Droid 2 and HTC Desire A8181
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.