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

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Adam Moore]

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 1

[Adam Moore]

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

Comment 2

[Scoobidiver (away)]

Please provide a testcase as an attachment.

Comment 3

[Adam Moore]

Attachment: 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.

Comment 4

[Loic]

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

Suspected bug 832923.

Comment 5

[Loic]

Why are the CR not connected to this bug report?

Comment 6

[Adam Moore]

@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]

Not to you.

Comment 8

[Scoobidiver (away)]

(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]

How much of this can we estimate is on top sites?  Is it 100% reproducible?

Comment 10

[Adam Moore]

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]]

Attachment: patch

This fixes the crash. But still the add-ons have to be updated to the new interface.

Comment 12

[Alex Keybl [:akeybl]]

I would like to land this in today's beta, if at all possible.

Comment 13

[Jorge Villalobos [:jorgev] (he/him)]

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]

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] (he/him)]

Is the proprietary code using a binary component to override nsIFilePicker?

Comment 16

[Adam Moore]

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 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6f5f334a15

Can we get a regression test for this?

Comment 19

[Alex Keybl [:akeybl]]

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.

Comment 20

[Jorge Villalobos [:jorgev] (he/him)]

(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 [Ex-Mozilla]]

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.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for suspected bustage and re-landed once cleared.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b4cad5ad857

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b4cad5ad857

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a991fa3810ac
https://hg.mozilla.org/releases/mozilla-beta/rev/c7ae9b066608

Comment 25

[Manuela Muntean [Away]]

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 26

[Manuela Muntean [Away]]

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

Comment 27

[Andrea Marchesini [:baku]]

> 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] (he/him)]

Yes, I'm okay with this approach, since the objective was just to get rid of the crash.

Comment 29

[Manuela Muntean [Away]]

Marking this VERIFIED FIXED for Fx 22, based on comment 25, comment 27 and comment 28.

Comment 30

[u279076]

Manuela, can you please recheck this with Firefox 23 and 24?

Comment 31

[Manuela Muntean [Away]]

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.