Closed
Bug 744910
Opened 12 years ago
Closed 12 years ago
Remove FileException from workers
Categories
(Core :: DOM: Workers, defect)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: Ms2ger, Assigned: emk)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 9 obsolete files)
36.55 KB,
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
As in bug 730161
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
rebased to tip
Attachment #615110 -
Attachment is obsolete: true
Attachment #615110 -
Flags: review?(bent.mozilla)
Attachment #615114 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 3•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
Assignee | ||
Comment 7•12 years ago
|
||
- 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•12 years ago
|
||
Assignee | ||
Comment 10•12 years ago
|
||
Sorry, wrong file attached.
Attachment #617293 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
(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•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 14•12 years ago
|
||
Added dev-doc-needed, but I'm not sure if there is any consequence for developers. Any hing?
Comment 15•12 years ago
|
||
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•12 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•12 years ago
|
||
Rebased to tip
Attachment #617294 -
Attachment is obsolete: true
Attachment #617295 -
Attachment is obsolete: true
Attachment #618245 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Whiteboard: [autoland-try:-b do -p all -u all -t none]
Comment 18•12 years ago
|
||
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
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a0bc511b1d75
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 20•12 years ago
|
||
Backed out: https://hg.mozilla.org/mozilla-central/rev/b96c764c00b2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•12 years ago
|
Whiteboard: [autoland-try:-b do -p all -u all -t none] → [autoland-in-queue]
Comment 21•12 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•12 years ago
|
Whiteboard: [autoland-in-queue]
https://hg.mozilla.org/mozilla-central/rev/c243a9cc89d3
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•