Closed Bug 923310 Opened 12 years ago Closed 12 years ago

Crash in HTMLInputElement in B2G

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27
Project Flags:
blocking-b2g koi+
Tracking Flags:
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)

interdiff
3.82 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
bug.patch
9.32 KB, patch
Details | Diff | Splinter Review
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 interdiff — — Splinter Review
There is an issue with a mochitest. This interdiff fixes it.
Attachment #814349 - Flags: review?(jwatt)
Attached patch bug.patch — — Splinter 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: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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)]
Keywords: crash, regression, reproducible
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
status-b2g-v1.2: --- → fixed
status-firefox25: --- → wontfix
status-firefox26: --- → fixed
status-firefox27: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: