Closed Bug 1102477 Opened 7 years ago Closed 6 years ago

MozLoopApi#cloneValueInto doesn't work with Exceptions

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
1

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37
Iteration:
37.3 - 12 Jan
Blocking Flags:
backlog Fx37?

People

(Reporter: standard8, Assigned: mikedeboer)

References

Details

(Whiteboard: [tech-debt])

Attachments

(2 files, 2 obsolete files)

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.
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
We can use value instanceof Components.Exception to detect the type, then just have to clone it.
(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?
(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).
(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?
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)
Attachment #8538502 - Flags: review?(standard8)
Added to IT 37.2
Flags: qe-verify?
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Flags: qe-verify? → qe-verify-
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.
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-
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.
Right, good point :)

So I think the check should indeed be:
```js
if (("error" in value) && (value.error instanceof Ci.nsIException)) {
  value = value.error;
}
```
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)
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)
Iteration: 37.2 → 37.3
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)
Attachment #8539223 - Attachment is obsolete: true
Attachment #8539223 - Flags: review?(standard8)
Attachment #8539223 - Flags: review?(jaws)
Attachment #8540738 - Flags: review?(standard8)
Attachment #8540738 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/d350cd94c551
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.