MozLoopApi#cloneValueInto doesn't work with Exceptions

RESOLVED FIXED in mozilla37

Status

Hello (Loop)
Client
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: standard8, Assigned: mikedeboer)

Tracking

unspecified
mozilla37
Points:
1
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tech-debt])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 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

4 years ago
We can use value instanceof Components.Exception to detect the type, then just have to clone it.
(Assignee)

Comment 3

4 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

4 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

4 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.
backlog: --- → Fx37?
(Assignee)

Comment 6

4 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

4 years ago
Created attachment 8538502 [details] [diff] [review]
Patch v1: pass HAWK request errors through to content
Attachment #8538502 - Flags: review?(standard8)
Added to IT 37.2
Flags: qe-verify?
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify-
(Reporter)

Comment 9

4 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

4 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

4 years ago
Created attachment 8538767 [details] [diff] [review]
Failure case example

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

4 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

4 years ago
Created attachment 8539223 [details] [diff] [review]
Patch v2: pass HAWK request errors through to content

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

4 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

4 years ago
Iteration: 37.2 → 37.3
(Assignee)

Comment 15

4 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

4 years ago
Created attachment 8540738 [details] [diff] [review]
Patch v2.1: pass HAWK request errors through to content
Attachment #8539223 - Attachment is obsolete: true
Attachment #8539223 - Flags: review?(standard8)
Attachment #8539223 - Flags: review?(jaws)
Attachment #8540738 - Flags: review?(standard8)
(Reporter)

Updated

4 years ago
Attachment #8540738 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/d350cd94c551
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.