Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove FileException from workers

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Ms2ger, Assigned: emk)

Tracking

({dev-doc-complete})

Trunk
mozilla15
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

5 years ago
As in bug 730161
(Assignee)

Updated

5 years ago
Depends on: 742191
(Assignee)

Comment 1

5 years ago
Created attachment 615110 [details] [diff] [review]
patch
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #615110 - Flags: review?(bent.mozilla)
(Assignee)

Comment 2

5 years ago
Created attachment 615114 [details] [diff] [review]
patch

rebased to tip
Attachment #615110 - Attachment is obsolete: true
Attachment #615110 - Flags: review?(bent.mozilla)
Attachment #615114 - Flags: review?(bent.mozilla)
(Assignee)

Comment 3

5 years ago
Created attachment 615127 [details] [diff] [review]
patch

Forgot to add a return statement after JS_ReportError.
Attachment #615114 - Attachment is obsolete: true
Attachment #615114 - Flags: review?(bent.mozilla)
Attachment #615127 - Flags: review?(bent.mozilla)
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.
(Assignee)

Comment 5

5 years ago
Created attachment 616328 [details] [diff] [review]
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?
Attachment #615127 - Attachment is obsolete: true
Attachment #615127 - Flags: review?(bent.mozilla)
Attachment #616328 - Flags: review?(bent.mozilla)
(Assignee)

Comment 6

5 years ago
Created attachment 616329 [details] [diff] [review]
Interdiff from the previous patch (for ease of review)
(Assignee)

Comment 7

5 years ago
Created attachment 616623 [details] [diff] [review]
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.
Attachment #616328 - Attachment is obsolete: true
Attachment #616328 - Flags: review?(bent.mozilla)
Attachment #616623 - Flags: review?(bent.mozilla)
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.
(Assignee)

Comment 9

5 years ago
Created attachment 617293 [details] [diff] [review]
interdiff v2.1 -> v3
(Assignee)

Comment 10

5 years ago
Created attachment 617294 [details] [diff] [review]
interdiff v2.1 -> v3

Sorry, wrong file attached.
Attachment #617293 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 617295 [details] [diff] [review]
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
Attachment #616329 - Attachment is obsolete: true
Attachment #616623 - Attachment is obsolete: true
Attachment #616623 - Flags: review?(bent.mozilla)
Attachment #617295 - Flags: review?(bent.mozilla)
Comment on attachment 617295 [details] [diff] [review]
patch v3

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

Looks great! Thanks!
Attachment #617295 - Flags: review?(bent.mozilla) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0bc511b1d75
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
Keywords: dev-doc-needed
Added dev-doc-needed, but I'm not sure if there is any consequence for developers. Any hing?
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.
Target Milestone: mozilla15 → ---
(Reporter)

Comment 16

5 years ago
(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.
(Assignee)

Comment 17

5 years ago
Created attachment 618245 [details] [diff] [review]
patch v3.1

Rebased to tip
Attachment #617294 - Attachment is obsolete: true
Attachment #617295 - Attachment is obsolete: true
Attachment #618245 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t none]
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
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/a0bc511b1d75
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Backed out: https://hg.mozilla.org/mozilla-central/rev/b96c764c00b2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

5 years ago
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]

Comment 21

5 years ago
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.

Updated

5 years ago
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/mozilla-central/rev/c243a9cc89d3
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.