Closed Bug 1331705 Opened 4 years ago Closed 4 years ago
Do not use an XRay-ed Promise during recipe execution
59 bytes, text/x-review-board-request
This comes from https://github.com/mozilla/normandy-addon/pull/43 From that PR: > The RecipeRunner uses a promise from the sandbox to detect when > execution is done, but the promise methods aren't available since the > object is XRay-ed and its prototype is set to Object. > > Instead, this patch uses two new functions passed to the sandbox that > map to resolving or rejecting a promise. The new executeAction function > returns a Promise defined outside the sandbox, but resolved or rejected > from within it when execution is finished. > > The Promise also makes testing a bit easier as we can now inspect the > value that the sandbox resolves with.
Gijs: You reviewed this in GitHub, but I wasn't sure if I should tag it for review in a bug before landing it, so I'm erring on the side of more review.
Comment on attachment 8827581 [details] Bug 1331705 - shield-recipe-client: Do not use an XRay-ed Promise during recipe execution, https://reviewboard.mozilla.org/r/105210/#review106108 Yeah, I think you can just land things if I r+'d on github. See also some more detail on my thoughts about this in https://github.com/mozilla/normandy/pull/428#issuecomment-273334894 . (I'm just assuming this is a straight port and not looking at this again. :-)
Attachment #8827581 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/ce90a9d52e86 shield-recipe-client: Do not use an XRay-ed Promise during recipe execution, r=Gijs
I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=69746678&repo=autoland https://hg.mozilla.org/integration/autoland/rev/d5479d988aff
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/a506d21c911e shield-recipe-client: Do not use an XRay-ed Promise during recipe execution, r=Gijs
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.