The default bug view has changed. See this FAQ.

file[multiple] form field nsIFilePicker crash if overriding component has not implemented a domfiles getter (Firefox 22+)

VERIFIED FIXED in Firefox 22

Status

()

Core
DOM: Core & HTML
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Adam Moore, Assigned: baku)

Tracking

(6 keywords)

22 Branch
mozilla24
x86
All
addon-compat, crash, regression, reproducible, testcase, verifyme
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ verified, firefox23+ verified, firefox24+ verified)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.20 Safari/537.36

Steps to reproduce:

Ran an existing extension, which has an nsIFilePicker compont, in Firefox 22.0b3.  It has worked fine for some time, but stopped working early in the FF22 dev cycle.


Actual results:

When attempting an upload to a multiple file form field, the browser crashes when the aFilePickerShownCallback provided to the open method is executed.  Single file upload attempts fail silently.


Expected results:

Happy, functional file picker -- or at least one that doesn't crash the browser and gives me a hint at what is wrong. This looks like a result of a form field implementation refactoring for B2G.  The internals are relying on the domfile and domfiles getters instead of the file and files getters, and fails badly if the new getters are missing.  Doesn't seem like this change was necessary at all, but I'm sure you had your reasons.   It would be nice if this was listed in the developer notes for Firefox 22.

This is easily reproducible by removing the domfile and domfiles getters from SpecialPowers.MockFilePicker
(Reporter)

Updated

4 years ago
OS: Mac OS X → All
(Reporter)

Comment 1

4 years ago
Crash report: https://crash-stats.mozilla.com/report/index/e1737257-9b1b-4fc7-85a0-d79e82130530

Updated

4 years ago
Severity: normal → critical
Crash Signature: [@ mozilla::dom::HTMLInputElement::nsFilePickerShownCallback::Done(short)]
tracking-firefox22: --- → ?
Component: Untriaged → DOM: Core & HTML
Keywords: crash
Product: Firefox → Core

Comment 2

4 years ago
Please provide a testcase as an attachment.
Keywords: regression, regressionwindow-wanted, testcase-wanted

Updated

4 years ago
Flags: needinfo?(am5050)
(Reporter)

Comment 3

4 years ago
Created attachment 759243 [details]
Test file picker component, works in FF21, crashes FF22

I simply removed the two new getters from MockFilePicker.jsm to reproduce the issue.  This works as expected in FF21 (auto-selects a random file), and crashes FF22 when used on a multiple file form field, e.g., http://encodable.com/uploaddemo/

The change in behavior happened here:

https://github.com/mozilla/releases-mozilla-central/commit/83fe0f7a99285941f9a497a9769b3d4a8f52a5e9

https://bugzilla.mozilla.org/show_bug.cgi?id=832923

All nsIFilePicker implementations that happened before this change will need to be updated in order to work.
Flags: needinfo?(am5050)

Updated

4 years ago
Attachment #759243 - Attachment mime type: application/octet-stream → application/x-xpinstall

Comment 4

4 years ago
Regression range:
good=2013-03-20
bad=2013-03-21
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8156df33b757&tochange=1d6fe70c79c5

Suspected bug 832923.
Blocks: 832923
Status: UNCONFIRMED → NEW
status-firefox21: --- → unaffected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
Ever confirmed: true
Keywords: regressionwindow-wanted, testcase-wanted → reproducible, testcase

Comment 5

4 years ago
Why are the CR not connected to this bug report?
(Reporter)

Comment 6

4 years ago
@Loic, if this was directed at me, I'm probably not aware of the conventions for this.  If CR is crash report, I created the bug via a link from one, and linked the report in comment 1 for good measure.

Comment 7

4 years ago
Not to you.

Comment 8

4 years ago
(In reply to Loic from comment #5)
> Why are the CR not connected to this bug report?
3:47AM UTC is not the right time to add a signature to a bug. See 865146.
How much of this can we estimate is on top sites?  Is it 100% reproducible?
Keywords: qawanted
(Reporter)

Comment 10

4 years ago
Any site that allows upload would be affected, so that is probably a large number -- but only if the user is running an extension that overrides the file picker component, and the extension has not been updated to deal with the interface change.
(Assignee)

Updated

4 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 11

4 years ago
Created attachment 760870 [details] [diff] [review]
patch

This fixes the crash. But still the add-ons have to be updated to the new interface.
Attachment #760870 - Flags: review?(Ms2ger)
I would like to land this in today's beta, if at all possible.
tracking-firefox22: ? → +
tracking-firefox23: ? → +
tracking-firefox24: ? → +
Keywords: addon-compat
Adam, were you intentionally trying to cause the crash with this add-on, or did you experience it with a different one that also overrides nsIFilePicker? It looks like all crash reports so far correspond to the sample you attached.
Flags: needinfo?(am5050)
(Reporter)

Comment 14

4 years ago
I found the crash while testing proprietary code, tracked down the root cause with a boiled-down test case.  The original crash report was from that test case, but I thought the MockFilePicker example would be more appropriate for the bug report.
Flags: needinfo?(am5050)
Is the proprietary code using a binary component to override nsIFilePicker?

Updated

4 years ago
Attachment #760870 - Flags: review?(Ms2ger) → review?(bugs)
(Reporter)

Comment 16

4 years ago
No, it is a script component similar to MockFilePicker, but it overrides nsIFilePicker in the manifest file.

component {e1430ca7-eea6-463b-b3f6-683b83bc390b} components/customfilepicker.js
contract @mozilla.org/filepicker;1 {e1430ca7-eea6-463b-b3f6-683b83bcd90b}
Comment on attachment 760870 [details] [diff] [review]
patch

I guess this should work to fix the crash.

But is there a bug to make nsIFilePicker interface saner.
After Bug 832923 it has files and domfiles, file and domfile, yet
files and file are useless.
Attachment #760870 - Flags: review?(bugs) → review+
(Assignee)

Updated

4 years ago
Blocks: 881801
(Assignee)

Updated

4 years ago
No longer blocks: 881801
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6f5f334a15

Can we get a regression test for this?
Flags: in-testsuite?
Keywords: checkin-needed
Comment on attachment 760870 [details] [diff] [review]
patch

Since we don't know the number of external add-ons relying upon this behavior, we'll want to take this fix speculatively on Aurora/Beta.
Attachment #760870 - Flags: approval-mozilla-beta+
Attachment #760870 - Flags: approval-mozilla-aurora+
(In reply to Adam Moore from comment #16)
> No, it is a script component similar to MockFilePicker, but it overrides
> nsIFilePicker in the manifest file.

I couldn't find any instances of this problem in the Add-ons MXR, but I'll post an update in the Add-ons Blog just in case.
Kairo brought up the XUL file picker as an area that should be tested. I downloaded the latest Linux tinderbox build. Set http://kb.mozillazine.org/Ui.allow_platform_file_picker to false.  I exercised all the UI elements on the XUL file picker window. I was able to use the XUL file picker to select images and other files on 3 different sites that allow uploading files.
Backed out for suspected bustage and re-landed once cleared.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4cad5ad857
https://hg.mozilla.org/mozilla-central/rev/9b4cad5ad857
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox24: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
https://hg.mozilla.org/releases/mozilla-aurora/rev/a991fa3810ac
https://hg.mozilla.org/releases/mozilla-beta/rev/c7ae9b066608
status-firefox22: --- → fixed
status-firefox23: --- → fixed
I was able to reproduce this issue with Firefox 22 beta 3 on Mac OSX 10.8.3 in 32bit mode, following comment 3. (Firefox crashed right after pressing the "Browse" button).

But after following the exact steps with Firefox 22 beta 5 (build ID: 20130612084701, the browser doesn't crash anymore indeed, but the "Browse" button doesn't seem to work either. It doesn't auto-select a random file, like it does in Firefox 21 (after installing the add-on). This behavior happens on both the Mac machine and an Ubuntu 12.10 32bit one.

Does anyone have any suggestions? Thanks!
Flags: needinfo?
Socorro reports from the last month, regarding the signature of this bug are available here:

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=mozilla%3A%3Adom%3A%3AHTMLInputElement%3A%3AnsFilePickerShownCallback%3A%3ADone%28short%29&reason_type=contains&date=06%2F13%2F2013%2009%3A03%3A49&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozilla%3A%3Adom%3A%3AHTMLInputElement%3A%3AnsFilePickerShownCallback%3A%3ADone%28short%29
(Assignee)

Comment 27

4 years ago
> Does anyone have any suggestions? Thanks!

The test implements just part of the current nsIFilePicker interface. This means that HTMLInputElement is not able to retrieve the list of files from this filePicker component.

I think this is the right behaviour. I don't think we should be retro-compatible with the previous interface. jorgev, feedback?
Flags: needinfo? → needinfo?(jorge)
Yes, I'm okay with this approach, since the objective was just to get rid of the crash.
Flags: needinfo?(jorge)
Marking this VERIFIED FIXED for Fx 22, based on comment 25, comment 27 and comment 28.
status-firefox22: fixed → verified
QA Contact: manuela.muntean
Manuela, can you please recheck this with Firefox 23 and 24?
Keywords: qawanted → verifyme
Verified fixed with both Fx 23 (latest Aurora, build ID: 20130618004018) and Fx 24 (latest Nightly, build ID: 20130618031335) on Mac OSX 10.8.3 in 32bit mode and Ubuntu 12.10 32bit, with the same results as in comment 25.
Status: RESOLVED → VERIFIED
status-firefox23: fixed → verified
status-firefox24: fixed → verified
You need to log in before you can comment on or make changes to this bug.