1059754 - Propagating errors to content with cloneInto fails in MozLoopAPI

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[:Paolo Amadini]

The following code in MozLoopAPI.jsm fails to propagate the error to content:

return MozLoopService.hawkRequest(path, method, payloadObj).then((response) => {
  callback(null, response.body);
}, (error) => {
  callback(Cu.cloneInto(error, targetWindow));
});

This was triggered simply by disconnecting from the network. Apparently, it is cloneInto that fails with the message:

Error: Encountered unsupported value type writing stack-scoped structured clone

Also, this error is reported asynchronously when the API is destroyed (missing ".catch(Cu.reportError);" at the end of the promise chain) making it difficult to find.

Comment 1

[Mark Banner (:standard8)]

This is probably a duplicate of bug 1055632, but in any case, I think Bug 1002416 has most of the fix for this already.

Comment 2

[:Paolo Amadini]

Attachment: The patch

It turns out that this bug resulted in the "Sorry, we were unable to retrieve a call url." message not being displayed. Apparently, this wasn't under automated test coverage.

The cloneInto call was actually unnecessary since the wrapping function does this automatically. Before realizing this, I made some edits to the wrapping code in preparation for reusing it. I've left them in the patch since I think they do improve readability.

Dan, are you the right reviewer for this change?

How can I add a regression test to ensure that the message above is displayed in case of errors?

Comment 3

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #2)
> The cloneInto call was actually unnecessary since the wrapping function does
> this automatically.

And this probably is not true, since injectObjectAPI is not called on the outer object. Now I wonder why the error propagated without the cloneInto call - maybe it just generated another error in the client?

Comment 4

[:Paolo Amadini]

I followed this all the way up to hawkclient.js. The original issue is probably that the client code may return an error object instead of an errorString:

http://mxr.mozilla.org/mozilla-central/source/services/common/hawkclient.js#209

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

This should handle the hawkError correctly, and also propagate error objects correctly upon registration.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8480613 [details] [diff] [review]
Updated patch

I confirmed that we do want this (the other patch doesn't fix it). I think someone like Mike would be better reviewing it.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8480613 [details] [diff] [review]
Updated patch

I've just been told that Mike is away, trying Mark instead.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8480613 [details] [diff] [review]
Updated patch

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

rs=me because it works, but I'd really appreciate having the comment below explained in the code, and/or a followup bug filed to actually figure this out.

::: browser/components/loop/MozLoopAPI.jsm
@@ +339,5 @@
> +          // an HTTP response status message, may also incorrectly be a native
> +          // error object that will cause the cloning function to fail.
> +          callback(Cu.cloneInto({
> +            error: (hawkError.error && typeof hawkError.error == "string")
> +                   ? hawkError.error : "Unexpected exception",

Why isn't this using cloneValueInto() ? Do we need to hide this from the caller?

Comment 9

[Mark Banner (:standard8)]

(In reply to :Gijs Kruitbosch from comment #8)
> Comment on attachment 8480613 [details] [diff] [review]
> Updated patch
> 
> Review of attachment 8480613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me because it works, but I'd really appreciate having the comment below
> explained in the code, and/or a followup bug filed to actually figure this
> out.
> 
> ::: browser/components/loop/MozLoopAPI.jsm
> @@ +339,5 @@
> > +          // an HTTP response status message, may also incorrectly be a native
> > +          // error object that will cause the cloning function to fail.
> > +          callback(Cu.cloneInto({
> > +            error: (hawkError.error && typeof hawkError.error == "string")
> > +                   ? hawkError.error : "Unexpected exception",
> 
> Why isn't this using cloneValueInto() ? Do we need to hide this from the
> caller?

Paolo, are you able to answer this?

Comment 10

[:Paolo Amadini]

We decided to go ahead and file a bug about the error inconsistency, which is bug 1063163.

Comment 11

[Mike de Boer [:mikedeboer]]

You can see http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/MozLoopAPI.jsm#62 how I handle errors as long as the Structured Cloning algorithm doesn't support Error objects.

You can ask :bholley for specifics and perhaps even a bug no. that you can track to see Error object support in the algo.

Comment 12

[Mike de Boer [:mikedeboer]]

More info and details in https://developer.mozilla.org/en-US/docs/Web/Guide/API/DOM/The_structured_clone_algorithm

Comment 13

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/9aae4c1be901

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9aae4c1be901

Comment 15

[u279076]

Untracking for QE verification. Please needinfo me to request testing.

Comment 16

[:Paolo Amadini]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #15)
> Untracking for QE verification. Please needinfo me to request testing.

Hi, here are steps to reproduce for this bug:
1. Activate the computer's network connection and start Firefox.
2. Open the Loop panel and wait for a call URL to be generated.
3. Close the Loop panel.
4. Deactivate the computer's network connection.
5. Open the Loop panel again and wait some time.

Expected results:
An error bar should be displayed with the message "Sorry, we were unable to retrieve a call url."

Actual results:
The panel waits for the URL forever.

Comment 17

[u279076]

Thanks Paolo. I'll make sure this is added to our verification queue. Does this need on-going smoketest coverage after verification or is it well enough covered?

Comment 18

[:Paolo Amadini]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #17)
> Thanks Paolo. I'll make sure this is added to our verification queue. Does
> this need on-going smoketest coverage after verification or is it well
> enough covered?

I think this test should be added to a list of manual tests, since it is not automated.

Comment 19

[u279076]

Paul, can you please verify this in the latest Nightly and make sure we get a Moztrap test? We'll skip verification in the Fig builds but it should have a test for 34 and 35 branch.

Comment 20

[Paul Silaghi, QA [:pauly]]

Verified fixed 35.0a1 (2014-09-30) Win 7, OS X 10.9.5, Ubuntu 13.04

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8480613 [details] [diff] [review]
Updated patch

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cf1a0a453a0

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8480613 [details] [diff] [review]
Updated patch

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 24

[u279076]

Paul, can you please test this in Aurora now that it's uplifted?

Comment 25

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b1 Ubuntu 13.04