Closed
Bug 1102477
Opened 10 years ago
Closed 9 years ago
MozLoopApi#cloneValueInto doesn't work with Exceptions
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(Not tracked)
backlog | Fx37? |
People
(Reporter: standard8, Assigned: mikedeboer)
References
Details
(Whiteboard: [tech-debt])
Attachments
(2 files, 2 obsolete files)
1.43 KB,
patch
|
Details | Diff | Splinter Review | |
3.06 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
I've been trying to debug some code in MozLoopAPI that is throwing an exception - trying to catch the exception and return it via a callback with cloneValueInto, I'm seeing: Error: Encountered unsupported value type writing stack-scoped structured clone This obviously makes debugging a bit harder, and means that we don't always get the callbacks called.
Assignee | ||
Comment 1•10 years ago
|
||
Please see bug 1102230, where I add a helper to aid you while developing. And hey, look, it's in your queue! :-P
Depends on: 1102230
Reporter | ||
Comment 2•10 years ago
|
||
We can use value instanceof Components.Exception to detect the type, then just have to clone it.
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #2) > We can use value instanceof Components.Exception to detect the type, then > just have to clone it. But the more interesting question, to me, is: why do we want chrome exceptions to cross the content boundary in the first place?
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #3) > (In reply to Mark Banner (:standard8) from comment #2) > > We can use value instanceof Components.Exception to detect the type, then > > just have to clone it. > > But the more interesting question, to me, is: why do we want chrome > exceptions to cross the content boundary in the first place? True, I think though, we need some consistent way of catching these and ensuring that a) it gets logged, b) the callback gets called with the fact there's been an error. Otherwise, we're liable to leave ourselves in bad states (like how missing room audio files in desktop, causes an exception, which then stops rooms from working).
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #4) > True, I think though, we need some consistent way of catching these and > ensuring that a) it gets logged, b) the callback gets called with the fact > there's been an error. Otherwise, we're liable to leave ourselves in bad > states (like how missing room audio files in desktop, causes an exception, > which then stops rooms from working). Again, see the little nugget I put in the patch for bug 1102230.
Updated•9 years ago
|
backlog: --- → Fx37?
Assignee | ||
Comment 6•9 years ago
|
||
I attached a patch to bug 1109149 before I noticed we have a bug for this already!
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 37.2
Points: --- → 1
Flags: needinfo?(mmucci)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8538502 -
Flags: review?(standard8)
Comment 8•9 years ago
|
||
Added to IT 37.2
Flags: qe-verify?
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Reporter | ||
Comment 9•9 years ago
|
||
From bug 1109149 comment 11: > I noticed this yesterday when testing Loop connection handling for bug > 1100149; HAWKClient uses rest.js to make HTTP requests and that bugger > yields nsIException objects inside a HAWK error object. > In other words; when the Loop server is down, content does get the error > that occurs when creating or deleting a room. This patch fixes that.
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8538502 [details] [diff] [review] Patch v1: pass HAWK request errors through to content Review of attachment 8538502 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/MozLoopAPI.jsm @@ +79,5 @@ > } > > + // HAWK request errors contain an nsIException object inside `value`. > + if (("error" in value) && ("code" in value) && ("errno" in value)) { > + value = value.error; I think this is wrong - not all HAWK request errors contain an nsIExpection object. Maybe check for: ("error" in value) && (value instanceof Ci.nsIException) I'll attach a test case in a moment for what I've just tried that confirmed to me that this isn't quite right.
Attachment #8538502 -
Flags: review?(standard8) → review-
Reporter | ||
Comment 11•9 years ago
|
||
This is the failure case I applied - our old friend the l10n of room names. With both your patch and this one, when I hit start conversation, I get: "Failed to clone value:" TypeError: invalid dependency: error; expected Object, got String Stack trace: loop.validate</Validator.prototype._checkRequiredTypes/<@chrome://browser/content/loop/shared/js/validate.js:77:1 loop.validate</Validator.prototype._checkRequiredTypes@chrome://browser/content/loop/shared/js/validate.js:73:1 loop.validate</Validator.prototype.validate@chrome://browser/content/loop/shared/js/validate.js:61:7 Action@chrome://browser/content/loop/shared/js/actions.js:20:25 loop.store.RoomStore<.createRoom/<@chrome://browser/content/loop/shared/js/roomStore.js:280:31 Without your patch, I get the error generated on the panel just fine.
Assignee | ||
Comment 12•9 years ago
|
||
Right, good point :) So I think the check should indeed be: ```js if (("error" in value) && (value.error instanceof Ci.nsIException)) { value = value.error; } ```
Assignee | ||
Comment 13•9 years ago
|
||
Updated patch that also copies over more properties for nsIException objects, because they're apparently _not_ enumerable.
Attachment #8538502 -
Attachment is obsolete: true
Attachment #8539223 -
Flags: review?(standard8)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8539223 [details] [diff] [review] Patch v2: pass HAWK request errors through to content Redirecting to you, Jared... Mark is on PTO today.
Attachment #8539223 -
Flags: review?(standard8) → review?(jaws)
Updated•9 years ago
|
Iteration: 37.2 → 37.3
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8539223 [details] [diff] [review] Patch v2: pass HAWK request errors through to content This bug's been stale for a while... not important?
Attachment #8539223 -
Flags: review?(standard8)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8539223 -
Attachment is obsolete: true
Attachment #8539223 -
Flags: review?(standard8)
Attachment #8539223 -
Flags: review?(jaws)
Attachment #8540738 -
Flags: review?(standard8)
Reporter | ||
Updated•9 years ago
|
Attachment #8540738 -
Flags: review?(standard8) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/d350cd94c551
https://hg.mozilla.org/mozilla-central/rev/d350cd94c551
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•