Closed Bug 1259588 Opened 4 years ago Closed 4 years ago

new File("") throws Gecko internal NS_ERROR_FAILURE

Categories

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

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: smaug, Assigned: shawnjohnjr)

Details

(Whiteboard: n[tw-dom] btpp-active)

Attachments

(1 file, 2 obsolete files)

If it should throw per spec, it should throw some correct exception.
Assignee: nobody → shuang
Whiteboard: [tw-dom] → [tw-dom] btpp-active
It looks like it's calling File constructor for chrome process only[1][2].

I don't see File API specification defines exception that I shall throw in this case. And Chromium throws TypeError exception.

[1]https://dxr.mozilla.org/mozilla-central/source/dom/webidl/File.webidl?case=true&from=File.webidl#15
[2]https://dxr.mozilla.org/mozilla-central/source/dom/base/File.cpp?case=true&from=File.cpp#625

Is it correct to throw TypeError exception if the File API specification doesn't define it?
Flags: needinfo?(bugs)
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#ChromeConstructor
Maybe we shall use '[ChromeConstructor]' in File.webidl? Or there is any historical reason we need to expose these constructor?
(In reply to Shawn Huang [:shawnjohnjr] from comment #2)
> https://developer.mozilla.org/en-US/docs/Mozilla/
> WebIDL_bindings#ChromeConstructor
> Maybe we shall use '[ChromeConstructor]' in File.webidl? Or there is any
> historical reason we need to expose these constructor?

Sorry, I missed that [Constructor] and [ChromeConstructor] are mutually exclusive.
Shawn, what you want to do is replacing:

  aRv.Throw(NS_ERROR_FAILURE);

with: 

  return ThrowErrorMessage(cx, MSG_ILLEGAL_CONSTRUCTOR);
Flags: needinfo?(bugs)
(In reply to Andrea Marchesini (:baku) from comment #4)
> Shawn, what you want to do is replacing:
> 
>   aRv.Throw(NS_ERROR_FAILURE);
> 
> with: 
> 
>   return ThrowErrorMessage(cx, MSG_ILLEGAL_CONSTRUCTOR);
Thanks a lot for the hint. :)
I tried the suggestion but I saw "Tab crashed" after running the test case. Looking into the crash.


[Child 25866] WARNING: g_path_get_basename: assertion 'file_name != NULL' failed: 'glib warning', file /home/shawn/code/mozilla-inbound/toolkit/xre/nsSigHandlers.cpp, line 142
(process:25866): GLib-CRITICAL **: g_path_get_basename: assertion 'file_name != NULL' failed
JavaScript error: file:///home/shawn/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/bin/components/Weave.js, line 13: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXPCComponents_Utils.import]
0 INFO SimpleTest START
1 INFO TEST-START | dom/base/test/test_bug1259588.html
************ illegal constructor ***[Parent 25800] WARNING: pipe error (38): Connection reset by peer: file /home/shawn/code/mozilla-inbound/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 313
[Parent 25800] WARNING: pipe error (55): Connection reset by peer: file /home/shawn/code/mozilla-inbound/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 313
###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0074,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
#0  0x00007fffea33ebb8 in mozilla::dom::GetOrCreateDOMReflectorHelper<RefPtr<mozilla::dom::File>, true>::GetOrCreate (cx=0x7fffbfb4c400, value=..., 
    givenProto=givenProto@entry=..., rval=...) at /home/shawn/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h:1684
#1  0x00007fffea33031a in mozilla::dom::GetOrCreateDOMReflector<RefPtr<mozilla::dom::File> > (givenProto=..., rval=..., value=..., cx=<optimized out>)
    at /home/shawn/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/BindingUtils.h:1707
#2  mozilla::dom::FileBinding::_constructor (cx=0x7fffbfb4c400, argc=<optimized out>, vp=<optimized out>)
    at /home/shawn/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dom/bindings/FileBinding.cpp:1020
#3  0x00007fffeb8cd554 in js::CallJSNative (args=..., native=0x7fffea32f365 <mozilla::dom::FileBinding::_constructor(JSContext*, unsigned int, JS::Value*)>, cx=0x7fffbfb4c400)
    at /home/shawn/code/mozilla-inbound/js/src/jscntxtinlines.h:235
#4  js::CallJSNativeConstructor (args=..., native=0x7fffea32f365 <mozilla::dom::FileBinding::_constructor(JSContext*, unsigned int, JS::Value*)>, cx=0x7fffbfb4c400)
    at /home/shawn/code/mozilla-inbound/js/src/jscntxtinlines.h:268
#5  InternalConstruct (cx=0x7fffbfb4c400, args=...) at /home/shawn/code/mozilla-inbound/js/src/vm/Interpreter.cpp:581
#6  0x00007fffeb8bfa5c in Interpret (cx=0x7fffbfb4c400, state=...) at /home/shawn/code/mozilla-inbound/js/src/vm/Interpreter.cpp:2823
#7  0x00007fffeb8cbc8d in js::RunScript (cx=cx@entry=0x7fffbfb4c400, state=...) at /home/shawn/code/mozilla-inbound/js/src/vm/Interpreter.cpp:426
Why MSG_ILLEGAL_CONSTRUCTOR? It should be TypeError for non-content caller, because the first parameter can't be converted to a sequence
Whiteboard: [tw-dom] btpp-active → n[tw-dom] btpp-active
(In reply to Olli Pettay [:smaug] from comment #7)
> Why MSG_ILLEGAL_CONSTRUCTOR? It should be TypeError for non-content caller,
> because the first parameter can't be converted to a sequence

Oh! Right!
Comment on attachment 8742213 [details] [diff] [review]
Bug 1259588 - new File("") throws TypeError exception

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

::: dom/base/File.cpp
@@ +621,5 @@
>                    const ChromeFilePropertyBag& aBag,
>                    ErrorResult& aRv)
>  {
>    if (!nsContentUtils::ThreadsafeIsCallerChrome()) {
> +    aRv.ThrowTypeError<MSG_ILLEGAL_CONSTRUCTOR>();

This should be aRv.ThrowTypeError<MSG_MISSING_ARGUMENTS>(NS_LITERAL_STRING("File")).
Attachment #8742213 - Flags: review?(amarchesini) → review-
Comment on attachment 8742294 [details] [diff] [review]
Bug 1259588 - new File("") throws TypeError exception (v2)

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

Good!
Attachment #8742294 - Flags: review?(amarchesini) → review+
https://hg.mozilla.org/mozilla-central/rev/0a222ef3bf63
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.