Closed Bug 1059754 Opened 5 years ago Closed 5 years ago

Propagating errors to content with cloneInto fails in MozLoopAPI

Categories

(Hello (Loop) :: Client, defect)

defect
Not set

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 1 open bug)

Details

(Whiteboard: [loop-uplift][fig:wontverify])

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 1002416
Attached patch The patch (obsolete) — Splinter Review
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?
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8480563 - Flags: review?(dmose)
(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
Attached patch Updated patchSplinter Review
This should handle the hawkError correctly, and also propagate error objects correctly upon registration.
Attachment #8480563 - Attachment is obsolete: true
Attachment #8480563 - Flags: review?(dmose)
Attachment #8480613 - Flags: review?(dmose)
Attachment #8480613 - Flags: review?(dmose) → review?(standard8)
Blocks: 1059186
No longer blocks: 1015988
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.
Attachment #8480613 - Flags: review?(standard8) → review?(mdeboer)
Comment on attachment 8480613 [details] [diff] [review]
Updated patch

I've just been told that Mike is away, trying Mark instead.
Attachment #8480613 - Flags: review?(mdeboer) → review?(mhammond)
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?
Attachment #8480613 - Flags: review?(mhammond) → review+
(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?
Flags: needinfo?(paolo.mozmail)
Depends on: 1063163
We decided to go ahead and file a bug about the error inconsistency, which is bug 1063163.
Flags: needinfo?(paolo.mozmail)
Depends on: 1065450
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.
https://hg.mozilla.org/mozilla-central/rev/9aae4c1be901
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Untracking for QE verification. Please needinfo me to request testing.
Flags: qe-verify-
QA Contact: anthony.s.hughes
Whiteboard: [loop-uplift?]
(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.
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(anthony.s.hughes)
Whiteboard: [loop-uplift?] → [loop-uplift]
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?
Flags: needinfo?(anthony.s.hughes) → needinfo?(paolo.mozmail)
(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.
Flags: needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paul.silaghi)
Flags: in-moztrap?
QA Contact: anthony.s.hughes → paul.silaghi
Whiteboard: [loop-uplift] → [loop-uplift][fig:wontverify]
Verified fixed 35.0a1 (2014-09-30) Win 7, OS X 10.9.5, Ubuntu 13.04
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Comment on attachment 8480613 [details] [diff] [review]
Updated patch

Approval Request Comment
Uplift request for patches staged and tested on Fig
Attachment #8480613 - Flags: approval-mozilla-aurora?
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.
Attachment #8480613 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Paul, can you please test this in Aurora now that it's uplifted?
Flags: in-moztrap? → needinfo?(paul.silaghi)
Verified fixed FF 34b1 Ubuntu 13.04
Flags: needinfo?(paul.silaghi)
You need to log in before you can comment on or make changes to this bug.