Last Comment Bug 880042 - file[multiple] form field nsIFilePicker crash if overriding component has not implemented a domfiles getter (Firefox 22+)
: file[multiple] form field nsIFilePicker crash if overriding component has not...
: addon-compat, crash, regression, reproducible, testcase, verifyme
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 22 Branch
: x86 All
: -- critical (vote)
: mozilla24
Assigned To: Andrea Marchesini [:baku]
: Manuela Muntean [Away]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 832923
  Show dependency treegraph
Reported: 2013-06-05 15:59 PDT by Adam Moore
Modified: 2013-06-18 23:55 PDT (History)
15 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Test file picker component, works in FF21, crashes FF22 (4.81 KB, application/x-xpinstall)
2013-06-06 10:18 PDT, Adam Moore
no flags Details
patch (1.07 KB, patch)
2013-06-11 06:23 PDT, Andrea Marchesini [:baku]
bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Adam Moore 2013-06-05 15:59:05 PDT
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
Comment 2 Scoobidiver (away) 2013-06-05 23:33:04 PDT
Please provide a testcase as an attachment.
Comment 3 Adam Moore 2013-06-06 10:18:17 PDT
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.,

The change in behavior happened here:

All nsIFilePicker implementations that happened before this change will need to be updated in order to work.
Comment 4 Loic 2013-06-06 14:59:56 PDT
Regression range:

Suspected bug 832923.
Comment 5 Loic 2013-06-06 15:01:53 PDT
Why are the CR not connected to this bug report?
Comment 6 Adam Moore 2013-06-06 16:57:51 PDT
@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 Loic 2013-06-06 17:20:58 PDT
Not to you.
Comment 8 Scoobidiver (away) 2013-06-07 00:31:00 PDT
(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.
Comment 9 Lukas Blakk [:lsblakk] use ?needinfo 2013-06-07 13:40:33 PDT
How much of this can we estimate is on top sites?  Is it 100% reproducible?
Comment 10 Adam Moore 2013-06-07 13:59:37 PDT
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.
Comment 11 Andrea Marchesini [:baku] 2013-06-11 06:23:54 PDT
Created attachment 760870 [details] [diff] [review]

This fixes the crash. But still the add-ons have to be updated to the new interface.
Comment 12 Alex Keybl [:akeybl] 2013-06-11 09:25:18 PDT
I would like to land this in today's beta, if at all possible.
Comment 13 Jorge Villalobos [:jorgev] 2013-06-11 09:35:10 PDT
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.
Comment 14 Adam Moore 2013-06-11 09:45:03 PDT
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.
Comment 15 Jorge Villalobos [:jorgev] 2013-06-11 09:49:52 PDT
Is the proprietary code using a binary component to override nsIFilePicker?
Comment 16 Adam Moore 2013-06-11 09:57:49 PDT
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;1 {e1430ca7-eea6-463b-b3f6-683b83bcd90b}
Comment 17 Olli Pettay [:smaug] 2013-06-11 10:09:31 PDT
Comment on attachment 760870 [details] [diff] [review]

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.
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-06-11 12:11:21 PDT

Can we get a regression test for this?
Comment 19 Alex Keybl [:akeybl] 2013-06-11 13:09:17 PDT
Comment on attachment 760870 [details] [diff] [review]

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.
Comment 20 Jorge Villalobos [:jorgev] 2013-06-11 13:16:50 PDT
(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.
Comment 21 Kevin Brosnan [:kbrosnan] 2013-06-11 17:52:51 PDT
Kairo brought up the XUL file picker as an area that should be tested. I downloaded the latest Linux tinderbox build. Set 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.
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-06-11 17:53:32 PDT
Backed out for suspected bustage and re-landed once cleared.
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-06-11 18:09:20 PDT
Comment 25 Manuela Muntean [Away] 2013-06-13 02:16:21 PDT
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!
Comment 27 Andrea Marchesini [:baku] 2013-06-13 03:14:06 PDT
> 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?
Comment 28 Jorge Villalobos [:jorgev] 2013-06-13 15:27:22 PDT
Yes, I'm okay with this approach, since the objective was just to get rid of the crash.
Comment 29 Manuela Muntean [Away] 2013-06-13 23:34:19 PDT
Marking this VERIFIED FIXED for Fx 22, based on comment 25, comment 27 and comment 28.
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-06-17 16:02:51 PDT
Manuela, can you please recheck this with Firefox 23 and 24?
Comment 31 Manuela Muntean [Away] 2013-06-18 23:55:23 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.