Closed Bug 1076967 Opened 10 years ago Closed 10 years ago

Fix Error object data propagation to Loop content pages

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
normal
Points:
1

Tracking

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

RESOLVED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox33 --- wontfix
firefox34 + fixed
firefox35 + fixed
firefox36 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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-
(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)
Attachment #8498974 - Attachment is obsolete: true
Attachment #8504638 - Flags: review?(bobbyholley)
Attachment #8504638 - Flags: review?(bobbyholley) → review+
Thanks, Bobby!

Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/14cfea6cd6ae
Iteration: 35.3 → 36.1
https://hg.mozilla.org/mozilla-central/rev/14cfea6cd6ae
Status: ASSIGNED → RESOLVED
Closed: 10 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
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+
Tested and good on Aurora nightly build; windows and linux.
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.