744910 - Remove FileException from workers

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

As in bug 730161

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: patch

rebased to tip

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch

Forgot to add a return statement after JS_ReportError.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 615127 [details] [diff] [review]
patch

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

::: dom/workers/Exceptions.cpp
@@ +138,2 @@
>  
> +    JSString *colon = JS_NewStringCopyZ(aCx, ": ");

You already know the length, save a strlen by passing it to JS_NewStringCopyN.

@@ +138,5 @@
>  
> +    JSString *colon = JS_NewStringCopyZ(aCx, ": ");
> +    if (!colon) {
> +      return false;
> +    }

Nit: newline here please

@@ +145,5 @@
>        return false;
>      }
>  
> +    jsval message = JS_GetReservedSlot(obj, SLOT_message);
> +    JS_ASSERT(message.isString());

Nit: newline here too

@@ +146,5 @@
>      }
>  
> +    jsval message = JS_GetReservedSlot(obj, SLOT_message);
> +    JS_ASSERT(message.isString());
> +    out = JS_ConcatStrings(aCx, out, JSVAL_TO_STRING(message));

Nit: message.toString() here too.

@@ -203,5 @@
>  #define EXCEPTION_ENTRY(_name) \
>    { #_name, _name, CONSTANT_FLAGS, GetConstant, NULL },
>  
> -  // Make sure this one is always first.
> -  EXCEPTION_ENTRY(UNKNOWN_ERR)

Hm, I don't think this is ok... It means that non-DOM errors won't generate an exception at all, right? We have lots of code (in XHR for example) where we pass return values straight through from necko or elsewhere and I think we need a fallback path for those.

@@ +250,5 @@
>    }
>  
> +  const char* name;
> +  const char* message;
> +  PRUint16 code;

Nit: uint16_t

@@ +294,3 @@
>  {
> +  JSObject* exception = DOMException::Create(aCx, aNSResult);
> +  if (!exception) {

This isn't really right since Create now returns NULL for failures in xpcom as well as OOM. In the OOM case you don't want to report an additional error.

Also, doing JS_ReportError for unknown codes basically means that people have to do much smarter exception handling since the exception could be a DOMException or it could be just an Error object.

::: dom/workers/FileReaderSync.cpp
@@ +69,5 @@
>    if (NS_SUCCEEDED(rv)) {
>      return true;
>    }
>  
> +  rv = (rv == NS_ERROR_FILE_NOT_FOUND) ?

Nit: Please remove extra parens

::: dom/workers/Makefile.in
@@ +44,5 @@
>  
>  # Public stuff.
> +EXPORTS_mozilla/dom/workers = \
> +  Workers.h \
> +  Exceptions.h \

Why is this needed? I'd really like to avoid this.

::: dom/workers/test/xhr2_worker.js
@@ +66,2 @@
>        throw new Error("Failed to throw when getting responseText on '" + type +
>                        "' type");

You're testing three conditions and giving the same failure message for each, which will mislead people seeing something like this on tinderbox. Please split into three checks.

@@ +96,5 @@
>    catch(e) {
>      exception = e;
>    }
>  
> +  if (!exception || exception.name != "InvalidStateError" || exception.code != DOMException.INVALID_STATE_ERR) {

Here too.

@@ +147,5 @@
>    catch(e) {
>      exception = e;
>    }
>  
> +  if (!exception || exception.name != "InvalidStateError" || exception.code != DOMException.INVALID_STATE_ERR) {

And here.

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: patch v2

(In reply to ben turner [:bent] from comment #4)
> @@ -203,5 @@
> >  #define EXCEPTION_ENTRY(_name) \
> >    { #_name, _name, CONSTANT_FLAGS, GetConstant, NULL },
> >  
> > -  // Make sure this one is always first.
> > -  EXCEPTION_ENTRY(UNKNOWN_ERR)
> 
> Hm, I don't think this is ok... It means that non-DOM errors won't generate
> an exception at all, right? We have lots of code (in XHR for example) where
> we pass return values straight through from necko or elsewhere and I think
> we need a fallback path for those.
It assumes that non-DOM errors will throw JS Error object by JS_ReportError (see below).

> @@ +294,3 @@
> >  {
> > +  JSObject* exception = DOMException::Create(aCx, aNSResult);
> > +  if (!exception) {
> 
> This isn't really right since Create now returns NULL for failures in xpcom
> as well as OOM. In the OOM case you don't want to report an additional error.
Added assertions again for OOM case.

> Also, doing JS_ReportError for unknown codes basically means that people
> have to do much smarter exception handling since the exception could be a
> DOMException or it could be just an Error object.
It is enough to check .name property. All unknown errors will return "Error". Web IDL discourages the use of "instanceof" because it is unreliable.
http://dev.w3.org/2006/webapi/WebIDL/#dfn-throw
Per Web IDL, the exception could be either a DOMException or a TypeError anyway (search for "TypeError" in the spec). Also, SVG spec may introduce RangeError as a replacement for some of SVG_INVALID_VALUE_ERR (see bug 743888).
Moreover, we already use "Error" (that is, JS_ReportError) everywhere in worker.
https://mxr.mozilla.org/mozilla-central/search?string=JS_ReportError&find=dom/workers

> >  # Public stuff.
> > +EXPORTS_mozilla/dom/workers = \
> > +  Workers.h \
> > +  Exceptions.h \
> 
> Why is this needed? I'd really like to avoid this.
Because I added a call to ThrowDOMExceptionForNSResult in bindings/Utils.h. So every bindings/Utils.h users will also need workers/Exceptions.h. Is there a better way to handle this situation?

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Interdiff from the previous patch (for ease of review)

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch v2.1

- Rebased to tip.
- Moved JS_ReportError into DOMException::Create because JS_ASSERT would have no effect in release builds.
- Moved ThrowDOMExceptionForNSResult declaration into Workers.h and stopped exporting Exceptions.h.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 616623 [details] [diff] [review]
patch v2.1

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

(In reply to Masatoshi Kimura [:emk] from comment #5)
> Moreover, we already use "Error" (that is, JS_ReportError) everywhere in
> worker.

This is true, but we're trying to use it less, not more. Whenever we see JS_ReportError we should be thinking about either improving the spec to handle such a failure or changing to one of the errors that the spec suggests. It's a long process and it's not any worse than our old UNKNOWN_ERR, so this is fine for now.

::: dom/bindings/Utils.h
@@ +17,5 @@
>  #include "XPCWrapper.h"
>  #include "nsTraceRefcnt.h"
>  #include "nsWrapperCacheInlines.h"
>  
> +using mozilla::dom::workers::exceptions::ThrowDOMExceptionForNSResult;

It's not ok to put a 'using' statement in a header like this. You can put it in the 'Throw' function though.

::: dom/workers/Exceptions.cpp
@@ +248,3 @@
>  {
>    JSObject* obj = JS_NewObject(aCx, &sClass, NULL, NULL);
> +  JS_ASSERT(obj);

This isn't something that you can assert. This could fail for several reasons (OOM is the most obvious) and none of them should be fatal. Please remove.

@@ +261,5 @@
>      return NULL;
>    }
>  
> +  JSString* jsname = JS_NewStringCopyZ(aCx, name);
> +  JS_ASSERT(jsname);

Same here.

@@ +267,5 @@
>      return NULL;
>    }
>  
> +  JSString* jsmessage = JS_NewStringCopyZ(aCx, message);
> +  JS_ASSERT(jsmessage);

And here.

::: dom/workers/File.cpp
@@ +156,5 @@
>      }
>  
>      PRUint64 size;
>      if (NS_FAILED(blob->GetSize(&size))) {
> +      ThrowDOMExceptionForNSResult(aCx, NS_ERROR_DOM_FILE_NOT_READABLE_ERR);

Uh, weird. This should have a 'return false' after here. Please add!

@@ +176,5 @@
>      }
>  
>      nsString type;
>      if (NS_FAILED(blob->GetType(type))) {
> +      ThrowDOMExceptionForNSResult(aCx, NS_ERROR_DOM_FILE_NOT_READABLE_ERR);

Here too.

::: dom/workers/Workers.h
@@ +96,5 @@
>  
>  // Random unique constant to facilitate JSPrincipal debugging
>  const uint32_t kJSPrincipalsDebugToken = 0x7e2df9d2;
>  
> +// Implemented in Exceptions.cpp

Nit: Move this to right before the function declaration.

::: dom/workers/XMLHttpRequest.cpp
@@ +1073,5 @@
>  
>      if (mMultipart) {
>        rv = mProxy->mXHR->SetMultipart(mMultipart);
>        if (NS_FAILED(rv)) {
> +        return rv;

Nit: Now that we don't have to call GetDOMExceptionCodeFromResult please replace these with NS_ENSURE_SUCCESS(rv, rv).

@@ +1080,5 @@
>  
>      if (mBackgroundRequest) {
>        rv = mProxy->mXHR->SetMozBackgroundRequest(mBackgroundRequest);
>        if (NS_FAILED(rv)) {
> +        return rv;

And here.

@@ +1087,5 @@
>  
>      if (mWithCredentials) {
>        rv = mProxy->mXHR->SetWithCredentials(mWithCredentials);
>        if (NS_FAILED(rv)) {
> +        return rv;

And here.

@@ +1094,5 @@
>  
>      if (mTimeout) {
>        rv = mProxy->mXHR->SetTimeout(mTimeout);
>        if (NS_FAILED(rv)) {
> +        return rv;

And here.

@@ +1167,5 @@
>        mBody.clear();
>        mClonedObjects.Clear();
>  
> +      if (NS_FAILED(rv)) {
> +        return rv;

And here.

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: interdiff v2.1 -> v3

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: interdiff v2.1 -> v3

Sorry, wrong file attached.

Comment 11

[Masatoshi Kimura [:emk]]

Attachment: patch v3

(In reply to ben turner [:bent] from comment #8)
> This is true, but we're trying to use it less, not more. Whenever we see
> JS_ReportError we should be thinking about either improving the spec to
> handle such a failure or changing to one of the errors that the spec
> suggests. It's a long process and it's not any worse than our old
> UNKNOWN_ERR, so this is fine for now.
This patch will call JS_ReportError only when UNKNOWN_ERROR was thrown formerly. Therefore it will make nothing worse because JS Error objects are not worse than UNKNOWN_ERROR as you said. Rather, it will throw a correct exception type in more cases than before (for example, exceptions thrown from Paris bindings).
Also Web IDL suggests using a predefined exception instead of minting a new exception type. Note that "exception type" means not a IDL exception but a JS .name property.
http://dev.w3.org/2006/webapi/WebIDL/#dfn-throw

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 617295 [details] [diff] [review]
patch v3

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

Looks great! Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0bc511b1d75

Comment 14

[Jean-Yves Perrier [:teoli]]

Added dev-doc-needed, but I'm not sure if there is any consequence for developers. Any hing?

Comment 15

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b96c764c00b2

because this changeset or another one from the same push caused 'make check' failures in Windows debug builds:
TEST-PASS | jit_test.py -m -d -n       | e:\builds\moz2_slave\m-in-w32-dbg\build\js\src\jit-test\tests\debug\Object-defineProperties-03.js
command timed out: 300 seconds without output, attempting to kill
https://tbpl.mozilla.org/php/getParsedLog.php?id=11178551&tree=Mozilla-Inbound

We can use the Try server to figure out whether this patch caused the errors.  If it does not cause the errors, we can re-land it.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jean-Yves Perrier [:teoli] from comment #14)
> Added dev-doc-needed, but I'm not sure if there is any consequence for
> developers. Any hing?

Yes: exception objects thrown from some File APIs in Web Workers will look somewhat different. I doubt that there are many pages that rely on the exact object that gets thrown, so I think a note on Fx# for devs will be enough.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment: patch v3.1

Rebased to tip

Comment 18

[Matt Brubeck (:mbrubeck)]

Backing out this patch did not fix the hangs, so I re-landed it.  (We ended up fixing the hangs by backing out some other patches.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c243a9cc89d3

Comment 19

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/a0bc511b1d75

Comment 20

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/b96c764c00b2

Comment 21

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 618245
	Branch: mozilla-central => try
Patch 618245 could not be applied to mozilla-central.
patching file dom/base/nsDOMException.cpp
Hunk #1 succeeded at 174 with fuzz 2 (offset 42 lines).
patching file dom/base/nsDOMException.h
Hunk #1 FAILED at 36
1 out of 1 hunks FAILED -- saving rejects to file dom/base/nsDOMException.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/c243a9cc89d3