Closed Bug 923310 Opened 6 years ago Closed 6 years ago

Crash in HTMLInputElement in B2G

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
blocking-b2g koi+
Tracking Status
firefox25 --- wontfix
firefox26 --- fixed
firefox27 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Keywords: crash, regression, reproducible, Whiteboard: [b2g-crash])

Crash Data

Attachments

(2 files, 2 obsolete files)

Attached patch bug.patch (obsolete) — Splinter Review
I don't understand why this didn't appear before. Maybe because I use a b2g Desktop build with a old gaia profile. The steps to reproduce this issue are:

1. open browser with a page with a <input type="file" />
2. use the wallpaper (maybe other apps are ok as well) and select a file.
3. b2g crashes here:

https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLInputElement.cpp#634

because file is null. File is null because of:

https://mxr.mozilla.org/mozilla-central/source/b2g/components/FilePicker.js#176
Attachment #813355 - Flags: review?(jwatt)
Comment on attachment 813355 [details] [diff] [review]
bug.patch

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

r- based on the nsDOMFileFile::GetMozFullPathInternal issue. Please clarify that and re-request review with the following issues addressed.

Please add a Doxygen style comment to  along the lines of:

/**
 * This may return nullptr if aDomFile's implementation of
 * nsIDOMFile::mozFullPathInternal does not successfully return a non-empty
 * string that is a valid path. This can happen on Firefox OS, for example,
 * where the file picker can create Blobs.
 */
static already_AddRefed<nsIFile>
DOMFileToLocalFile(nsIDOMFile* aDomFile)
{

::: content/base/src/nsDOMFile.cpp
@@ +495,5 @@
>  nsDOMFileFile::GetMozFullPathInternal(nsAString &aFilename)
>  {
> +  if (!mIsFile) {
> +    return NS_ERROR_FAILURE;
> +  }

I think you should undo this change. I don't see how we can fail this assertion, but if we can please explain how. If we can fail it, it would seem we have a bigger problem.

::: content/html/content/test/mochitest.ini
@@ +385,5 @@
>  [test_track_disabled.html]
>  [test_ul_attributes_reflection.html]
>  [test_undoManager.html]
>  [test_video_wakelock.html]
> +[test_wrongFile.html]

I think this would be better renamed to test_input_files_not_nsIFile.html

::: content/html/content/test/test_wrongFile.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for &lt;input type='file'&gt; with a wrong nsIFile</title>

Make this:

<title>Test for &lt;input type='file'&gt; handling when its "files" do not implement nsIFile</title>

::: testing/specialpowers/content/MockFilePicker.jsm
@@ +122,5 @@
>    parent: null,
>    filterIndex: 0,
>    displayDirectory: null,
>    get file() {
> +    if (MockFilePicker.returnFiles.length >= 1 &&

Add a comment along the lines of "window.File does not implement nsIFile" before this line.

@@ +131,5 @@
>    get domfile()  {
>      if (MockFilePicker.returnFiles.length >= 1) {
> +      if (MockFilePicker.returnFiles[0] instanceof MockFilePicker.window.File) {
> +        return MockFilePicker.returnFiles[0];
> +      } else {

Kill the |else| - the |if| ends with a |return|.

@@ +140,5 @@
>      }
>      return null;
>    },
>    get fileURL() {
> +    if (MockFilePicker.returnFiles.length >= 1 &&

Add a comment along the lines of "window.File does not implement nsIFile" before this line.

@@ +153,5 @@
>        hasMoreElements: function() {
>          return this.index < MockFilePicker.returnFiles.length;
>        },
>        getNext: function() {
> +        if (MockFilePicker.returnFiles[this.index] instanceof MockFilePicker.window.File) {

Add a comment along the lines of "window.File does not implement nsIFile" before this line.
Attachment #813355 - Flags: review?(jwatt) → review-
Attached patch bug.patch (obsolete) — Splinter Review
Attachment #813355 - Attachment is obsolete: true
Attachment #814318 - Flags: review?(jwatt)
Comment on attachment 814318 [details] [diff] [review]
bug.patch

Thanks!
Attachment #814318 - Flags: review?(jwatt) → review+
Attached patch interdiffSplinter Review
There is an issue with a mochitest. This interdiff fixes it.
Attachment #814349 - Flags: review?(jwatt)
Attached patch bug.patchSplinter Review
Attachment #814318 - Attachment is obsolete: true
Attachment #814349 - Flags: review?(jwatt) → review+
https://hg.mozilla.org/mozilla-central/rev/499b79679419
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Duplicate of this bug: 928166
koi+ per the blocker in the dupe - this will break twitter's upload photo functionality.
Blocks: 894840
blocking-b2g: --- → koi+
Crash Signature: [@ mozilla::dom::HTMLInputElement::nsFilePickerShownCallback::Done(short)]
Whiteboard: [b2g-crash]
Please can you attach an aurora patch:

[/c/src-gecko/aurora]$ transplant 499b79679419
searching for changes
applying 499b79679419
unable to find 'content/html/content/test/mochitest.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file content/html/content/test/mochitest.ini.rej
patch failed to apply
Whiteboard: [b2g-crash] → [b2g-crash] [needs-aurora-patch]
https://hg.mozilla.org/releases/mozilla-aurora/rev/e5d6daa4aa4c
Flags: in-testsuite+
Whiteboard: [b2g-crash] [needs-aurora-patch] → [b2g-crash]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.