Closed
Bug 1288480
Opened 5 years ago
Closed 5 years ago
Intermittent browser_webapi_install.js | Wrong error message: Promise rejection value is a non-unwrappable cross-compartment wrapper. - Got false, expected true
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: intermittent-bug-filer, Assigned: aswan)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file)
Filed by: rvandermeulen [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=220435&repo=ash http://archive.mozilla.org/pub/firefox/tinderbox-builds/ash-linux/1469111333/ash_ubuntu32_vm_test-mochitest-e10s-browser-chrome-2-bm03-tests1-linux32-build1.txt.gz
Comment 1•5 years ago
|
||
Maybe fallout from bug 911216 given the timing.
Comment 2•5 years ago
|
||
Given the frequency I'm seeing this on Ash, I strongly suspect that something got merged around that shouldn't have been.
Flags: needinfo?(wkocher)
Comment 3•5 years ago
|
||
Excitingly, this appears to be a bad interaction between bug 1287125 (which landed successfully on fx-team) and bug 911216 (which landed successfully on inbound), so the problem wasn't seen until the merges to m-c were done.
Assignee | ||
Comment 4•5 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66094/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66094/
Attachment #8773398 -
Flags: review?(kmaglione+bmo)
![]() |
||
Comment 5•5 years ago
|
||
https://reviewboard.mozilla.org/r/66094/#review62988 ::: toolkit/mozapps/extensions/amWebAPI.js:149 (Diff revision 1) > task(...args).then(wrapForContent) > - .then(resolve, reject); > + .then(resolve, details => { > + let err = new win.Error(details.message); > + Object.keys(details).forEach(name => { > + if (name != "message") { > + err[name] = details[name]; Why is it ok to expose details[name] to untrusted script if it's not a primitive?
![]() |
||
Comment 6•5 years ago
|
||
https://reviewboard.mozilla.org/r/66094/#review62990 ::: toolkit/mozapps/extensions/amWebAPI.js:146 (Diff revision 1) > } > > return new win.Promise((resolve, reject) => { > task(...args).then(wrapForContent) > - .then(resolve, reject); > + .then(resolve, details => { > + let err = new win.Error(details.message); For that matter, how do we know the message is safe to expose here? It seems to be coming from random possibly-unexpected exceptions in chrome code, and normally we go to great lengths to NOT expose such things to content.
Comment 7•5 years ago
|
||
https://reviewboard.mozilla.org/r/66094/#review62990 > For that matter, how do we know the message is safe to expose here? It seems to be coming from random possibly-unexpected exceptions in chrome code, and normally we go to great lengths to NOT expose such things to content. Agree. We should log this and report it as an unexpected exception unless we're certain it's an error we want to expose to content.
Updated•5 years ago
|
Attachment #8773398 -
Flags: review?(kmaglione+bmo) → review-
Comment 8•5 years ago
|
||
Comment on attachment 8773398 [details] Bug 1288480 Rework mozAddonManager error handling https://reviewboard.mozilla.org/r/66094/#review63004
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → aswan
Flags: needinfo?(aswan)
Assignee | ||
Comment 9•5 years ago
|
||
Comment on attachment 8773398 [details] Bug 1288480 Rework mozAddonManager error handling Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66094/diff/1-2/
Attachment #8773398 -
Attachment description: Bug 1288480 Create appropriate Errors for rejected webapi tasks → Bug 1288480 Rework mozAddonManager error handling
Attachment #8773398 -
Flags: review- → review?(bzbarsky)
Assignee | ||
Updated•5 years ago
|
Attachment #8773398 -
Flags: review?(bzbarsky) → review?(kmaglione+bmo)
Comment 10•5 years ago
|
||
Comment on attachment 8773398 [details] Bug 1288480 Rework mozAddonManager error handling https://reviewboard.mozilla.org/r/66094/#review63056 ::: toolkit/mozapps/extensions/amWebAPI.js:152 (Diff revision 2) > + if ("reject" in result) { > + let err = new win.Error(result.reject.message); > + // We don't currently put any other properties onto Errors > + // generated by mozAddonManager. If/when we do, they will > + // need to get copied here. > + reject(err); Should return here, or put the rest of the function body in an else. ::: toolkit/mozapps/extensions/amWebAPI.js:161 (Diff revision 2) > + if (boundProcessor) { > + obj = boundProcessor(obj); > + } > + resolve(obj); > + }).catch(err => { > + Cu.reportError(err.message); `Cu.reportError(err)`
Attachment #8773398 -
Flags: review?(kmaglione+bmo) → review+
Comment 11•5 years ago
|
||
Pushed by aswan@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/10501352b0ee Rework mozAddonManager error handling r=kmag a=kwierso CLOSED TREE
Assignee | ||
Comment 12•5 years ago
|
||
https://reviewboard.mozilla.org/r/66094/#review63056 > Should return here, or put the rest of the function body in an else. oops thanks for catching that
Comment 13•5 years ago
|
||
Pushed by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/e416493c5ebd ESLint fix a=me CLOSED TREE
Comment hidden (Intermittent Failures Robot) |
As far as I can see, this appears to have fixed the failures.
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment hidden (Intermittent Failures Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•