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)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: aswan)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1288480 Rework mozAddonManager error handling
58 bytes, text/x-review-board-request
kmag
: review+
Details 
Maybe fallout from bug 911216 given the timing.
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)
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.
Blocks: 911216, 1287125
Flags: needinfo?(wkocher) → needinfo?(aswan)
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?
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.
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.
Attachment #8773398 - Flags: review?(kmaglione+bmo) → review-
Assignee: nobody → aswan
Flags: needinfo?(aswan)
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)
Attachment #8773398 - Flags: review?(bzbarsky) → review?(kmaglione+bmo)
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+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/10501352b0ee
Rework mozAddonManager error handling r=kmag a=kwierso CLOSED TREE
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
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
You need to log in before you can comment on or make changes to this bug.