Fix Error object data propagation to Loop content pages

RESOLVED FIXED in Firefox 34

Status

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

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

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

Firefox Tracking Flags

(firefox33 wontfix, firefox34+ fixed, firefox35+ fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.35 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Created attachment 8498974 [details] [diff] [review]
Patch

I noticed earlier that Error object didn't contain the values I expected, so I asked bholley on IRC what could be the cause... a lengthy conversation followed and the end-result is this patch.
Attachment #8498974 - Flags: review?(bobbyholley)
Flags: qe-verify-
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to IT 35.3
Flags: needinfo?(mmucci)
Comment on attachment 8498974 [details] [diff] [review]
Patch

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +45,5 @@
>    for (let prop of Object.getOwnPropertyNames(error)) {
> +    Object.defineProperty(Cu.waiveXrays(obj), prop, {
> +      configurable: false,
> +      enumerable: true,
> +      value: String(error[prop])

This is wrong for things like line number, right?
Attachment #8498974 - Flags: review?(bobbyholley) → review-
(Assignee)

Comment 3

3 years ago
(In reply to Bobby Holley (:bholley) from comment #2)
> This is wrong for things like line number, right?

Why would that be wrong? The goal is to move the Error object past the chrome->content barrier as-is. The Error object from the targetWindow has its own lineNumber and fileName properties that'll always be the same for each error we pass, obscuring useful information from the original error object we're trying to clone, IF we don't overwrite these properties too.
Flags: needinfo?(bobbyholley)
(In reply to Mike de Boer [:mikedeboer] from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > This is wrong for things like line number, right?
> 
> Why would that be wrong?

Because you're stringifying an integer, so you'll end up with |"42"| instead of |42|.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 5

3 years ago
Created attachment 8504638 [details] [diff] [review]
Patch v1.1: fix Error object data propagation to Loop content pages
Attachment #8498974 - Attachment is obsolete: true
Attachment #8504638 - Flags: review?(bobbyholley)
Attachment #8504638 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 6

3 years ago
Thanks, Bobby!

Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/14cfea6cd6ae

Updated

3 years ago
Iteration: 35.3 → 36.1
https://hg.mozilla.org/mozilla-central/rev/14cfea6cd6ae
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8504638 [details] [diff] [review]
Patch v1.1: fix Error object data propagation to Loop content pages

Approval Request Comment
[Feature/regressing bug #]: Hello contact errors
[User impact if declined]: If there is a problem with Hello contacts, the user won't see any error messages in the browser console
[Describe test coverage new/current, TBPL]: landed in m-c
[Risks and why]: low risk to anything outside of Hello.  Without this the user has no way to know why a problem occurred
[String/UUID change made/needed]: No strings
Attachment #8504638 - Flags: approval-mozilla-beta?
Attachment #8504638 - Flags: approval-mozilla-aurora?
[Tracking Requested - why for this release]:
Requesting uplift - -See Comment 8
tracking-firefox34: --- → ?
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
status-firefox36: --- → fixed
tracking-firefox34: ? → +
tracking-firefox35: --- → +
Tested on Nightly 10/17 (so I could generate an error trying to import contacts) and works.
Comment on attachment 8504638 [details] [diff] [review]
Patch v1.1: fix Error object data propagation to Loop content pages

Thanks for testing jesup. Approved for Aurora. If everything goes well with your testing on Sunday, we'll get this uplifted for beta2.
Attachment #8504638 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/7907c5abdae0
status-firefox35: affected → fixed
Tested and good on Aurora nightly build; windows and linux.
https://hg.mozilla.org/releases/mozilla-beta/rev/880cfb4ef6f8
status-firefox34: affected → fixed
Comment on attachment 8504638 [details] [diff] [review]
Patch v1.1: fix Error object data propagation to Loop content pages

Previously approved offline. Adding approval to the bug.
Attachment #8504638 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.