1259588 - new File("") throws Gecko internal NS_ERROR_FAILURE

Status: RESOLVED FIXED

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

Description

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

If it should throw per spec, it should throw some correct exception.

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 4

[Andrea Marchesini [:baku]]

Shawn, what you want to do is replacing:

  aRv.Throw(NS_ERROR_FAILURE);

with: 

  return ThrowErrorMessage(cx, MSG_ILLEGAL_CONSTRUCTOR);

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

#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

Comment 7

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

Why MSG_ILLEGAL_CONSTRUCTOR? It should be TypeError for non-content caller, because the first parameter can't be converted to a sequence

Comment 8

[Andrea Marchesini [:baku]]

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

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1259588 - new File("") throws TypeError exception

Comment 10

[Andrea Marchesini [:baku]]

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")).

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1259588 - new File("") throws TypeError exception (v2)

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8742294 [details] [diff] [review]
Bug 1259588 - new File("") throws TypeError exception (v2)

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

Good!

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1259588 - new File(\"\") throws TypeError exception, r=baku

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e765a071a48

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a222ef3bf63

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a222ef3bf63