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.

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

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?

(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?

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

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

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 on attachment 8480613 [details] [diff] [review] Updated patch I've just been told that Mike is away, trying Mark instead.

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?

(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?

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

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.

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

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

(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.

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?

(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.

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.

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

Comment on attachment 8480613 [details] [diff] [review] Updated patch Approval Request Comment Uplift request for patches staged and tested on Fig

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.

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

Verified fixed FF 34b1 Ubuntu 13.04