Include better location information in errors triggered by async APIs

RESOLVED FIXED in Firefox 60

Status

enhancement
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(6 attachments)

Assignee

Description

a year ago
No description provided.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 7

a year ago
mozreview-review
Comment on attachment 8954243 [details]
Bug 1441333: Part 5 - Use proper async caller location in normalizeError.

https://reviewboard.mozilla.org/r/223396/#review229446

::: js/xpconnect/src/XPCJSRuntime.cpp:2903
(Diff revision 1)
>      JS_AddFinalizeCallback(cx, FinalizeCallback, nullptr);
>      JS_AddWeakPointerZonesCallback(cx, WeakPointerZonesCallback, this);
>      JS_AddWeakPointerCompartmentCallback(cx, WeakPointerCompartmentCallback, this);
>      JS_SetWrapObjectCallbacks(cx, &WrapObjectCallbacks);
>      js::SetPreserveWrapperCallback(cx, PreserveWrapper);
> +    JS_InitReadPrincipalsCallback(cx, nsJSPrincipals::ReadPrincipals);

Ignore this. It's from a patch in another bug, but accidentally got rolled into this one.

Comment 8

a year ago
mozreview-review
Comment on attachment 8954238 [details]
Bug 1441333: Part 1 - Add helper to retrieve closest stack frame with a given principal.

https://reviewboard.mozilla.org/r/223386/#review230096

::: js/xpconnect/tests/unit/test_getCallerLocation.js:4
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

nit: "use strict";

Comment 9

a year ago
mozreview-review
Comment on attachment 8954238 [details]
Bug 1441333: Part 1 - Add helper to retrieve closest stack frame with a given principal.

https://reviewboard.mozilla.org/r/223386/#review230406

r=me woth the nits below fixed.

::: dom/base/ChromeUtils.cpp:670
(Diff revision 1)
> +    JS_ClearPendingException(cx);
> +    return;

You need to explicitly assign null to aRetval in this case, I would think.

::: dom/webidl/ChromeUtils.webidl:302
(Diff revision 1)
> +
> +  /**
> +   * Returns the scripted location of the first ancestor stack frame with a
> +   * principal which subsumes the given principal.
> +   */
> +  object? getCallerLocation(Principal principal);

Please document what a null return value means (or can mean).

::: js/src/jsfriendapi.h:2992
(Diff revision 1)
>   * The savedFrame and cx do not need to be in the same compartment.
>   */
>  extern JS_FRIEND_API(JSObject*)
>  GetFirstSubsumedSavedFrame(JSContext* cx, JS::HandleObject savedFrame, JS::SavedFrameSelfHosted selfHosted);
>  
> +extern JS_FRIEND_API(JSObject*)

Needs to be documented.

::: js/src/vm/SavedStacks.cpp:576
(Diff revision 1)
> -// principals are subsumed by |principals|, according to |subsumes|. If there is
> -// no such frame, return nullptr. |skippedAsync| is set to true if any of the
> -// skipped frames had the |asyncCause| property set, otherwise it is explicitly
> -// set to false.
>  static SavedFrame*
> -GetFirstSubsumedFrame(JSContext* cx, HandleSavedFrame frame, JS::SavedFrameSelfHosted selfHosted,
> +GetFirstMatchedFrame(JSContext* cx, Matcher& matches,

Needs to be documented.

::: js/src/vm/SavedStacks.cpp:601
(Diff revision 1)
>  
>      return nullptr;
>  }
>  
> +// Return the first SavedFrame in the chain that starts with |frame| whose
> +// principals are subsumed by |principals|, according to |subsumes|. If there is

This comment doesn't make sense; we have no |principals| here.  Yes, that was a preexisting problem.


Presumably should be talking about being subsumed by the current principal of cx?
Attachment #8954238 - Flags: review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8954239 [details]
Bug 1441333: Part 2 - Allow passing an error stack to reportError.

https://reviewboard.mozilla.org/r/223388/#review230416

r=me modulo the two caveats below.

::: js/xpconnect/src/XPCComponents.cpp:2092
(Diff revision 1)
>      if (!scripterr) {
> +        RootedObject stackObj(cx);
> +        if (stack.isObject()) {
> +            stackObj = &stack.toObject();
> +
> +            if (GetSavedFrameLine(cx, stackObj, &lineNo) != SavedFrameResult::Ok) {

So this will do horrible things if stackObj is not a SavedFrame object.  Please test IsSavedFrame() before calling saved-frame APIs here.  Actually, before even assigning to stackObj.

I almost wonder whether it's worth using nsIStackFrame as the representation of these stacks.  It would be a tiny bit more overhead on capture (extra allocation), but make the code way simpler here.  For example, you coudld share it with the existing nsIStackFrame code.
Attachment #8954239 - Flags: review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8954240 [details]
Bug 1441333: Part 3 - Add helper to create a JS error with a saved stack.

https://reviewboard.mozilla.org/r/223390/#review230418

::: dom/base/ChromeUtils.cpp:700
(Diff revision 1)
> +      stack = UncheckedUnwrap(aStack);
> +      ac.emplace(cx, stack);
> +
> +      if (JS::GetSavedFrameLine(cx, stack, &line) != JS::SavedFrameResult::Ok ||

Something here needs to check that this is really a StackFrame.

::: dom/base/ChromeUtils.cpp:733
(Diff revision 1)
> +    MOZ_ASSERT(err.isObject());
> +    aRetVal.set(&err.toObject());
> +  }
> +
> +  if (aStack) {
> +    JS_WrapObject(cx, aRetVal);

This call can fail.
Attachment #8954240 - Flags: review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
Comment on attachment 8954240 [details]
Bug 1441333: Part 3 - Add helper to create a JS error with a saved stack.

https://reviewboard.mozilla.org/r/223390/#review230430
Attachment #8954240 - Flags: review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8954241 [details]
Bug 1441333: Part 4 - Add caller location to async API errors.

https://reviewboard.mozilla.org/r/223392/#review230104

::: toolkit/components/extensions/ExtensionCommon.jsm:287
(Diff revision 2)
>      } else {
>        try {
>          let {cloneScope} = this;
>          args = args.map(arg => Cu.cloneInto(arg, cloneScope));
>        } catch (e) {
>          Cu.reportError(e);

I think it makes sense to use the caller location for this error as well.

::: toolkit/components/extensions/ExtensionCommon.jsm:291
(Diff revision 2)
>        } catch (e) {
>          Cu.reportError(e);
>          dump(`runSafe failure: cloning into ${this.cloneScope}: ${e}\n\n${filterStack(Error())}`);
>        }
>  
>        return this.applySafeWithoutClone(callback, args);

Pass the caller here.

::: toolkit/components/extensions/ExtensionCommon.jsm:297
(Diff revision 2)
>      }
>    }
>  
> -  applySafeWithoutClone(callback, args) {
> +  applySafeWithoutClone(callback, args, caller) {
>      if (this.unloaded) {
>        Cu.reportError("context.runSafeWithoutClone called after context unloaded");

Missed this?  And below.

::: toolkit/components/extensions/ExtensionCommon.jsm:305
(Diff revision 2)
>      } else {
>        try {
>          return Reflect.apply(callback, null, args);
>        } catch (e) {
>          dump(`Extension error: ${e} ${e.fileName} ${e.lineNumber}\n[[Exception stack\n${filterStack(e)}Current stack\n${filterStack(Error())}]]\n`);
>          Cu.reportError(e);

Though we shouldn't use the caller here, if I'm understanding this correctly.

::: toolkit/components/extensions/ExtensionCommon.jsm:423
(Diff revision 2)
> +   * @param {SavedFrame} [caller]
> +   *        The optional caller frame which triggered this callback, to be used
> +   *        in error reporting.
>     * @returns {*} The return value of callback.
>     */
> -  withLastError(error, callback) {
> +  withLastError(error, callback, caller) {

Not thrilled about the param order. :/
Perhaps put callback first?
Attachment #8954241 - Flags: review?(tomica)
Assignee

Comment 20

a year ago
mozreview-review-reply
Comment on attachment 8954241 [details]
Bug 1441333: Part 4 - Add caller location to async API errors.

https://reviewboard.mozilla.org/r/223392/#review230104

> I think it makes sense to use the caller location for this error as well.

Nope. Passing the caller to reportError is only useful when we pass a string rather than an error object, which shouldn't happen here.

> Not thrilled about the param order. :/
> Perhaps put callback first?

Having the callback first makes the calls with inline functions hard to read. I suppose the caller could be the second argument, and we could pass null if we don't have one.

Comment 21

a year ago
mozreview-review
Comment on attachment 8954242 [details]
Bug 1441333: Part 6 - Use caller location in error reports for StrongPromise errors.

https://reviewboard.mozilla.org/r/223394/#review230464

r=me once the test passes.

::: toolkit/components/extensions/ExtensionChild.jsm:116
(Diff revision 2)
> -  observe(subject, topic, tag) {
> -    const pos = tag.indexOf("|");
> -    const channel = Number(tag.substr(0, pos));
> -    const location = tag.substr(pos + 1);
> -    const message = `Promised response from onMessage listener at ${location} went out of scope`;
> -    MessageChannel.abortChannel(channel, {message});
> +  observe(subject, topic, channelId) {
> +    channelId = Number(channelId);
> +    let location = this.locations.get(channelId);
> +    this.locations.delete(channelId);
> +
> +    const message = `Promised response from onMessage listener went out of scope`;

You likely need to update the test I added for this, I think it explicitly checks that location is present in the error message.
Attachment #8954242 - Flags: review?(tomica) → review+

Comment 22

a year ago
mozreview-review
Comment on attachment 8954243 [details]
Bug 1441333: Part 5 - Use proper async caller location in normalizeError.

https://reviewboard.mozilla.org/r/223396/#review230472

r=me with the test added.
Attachment #8954243 - Flags: review?(tomica) → review+

Comment 23

a year ago
mozreview-review-reply
Comment on attachment 8954241 [details]
Bug 1441333: Part 4 - Add caller location to async API errors.

https://reviewboard.mozilla.org/r/223392/#review230104

> Nope. Passing the caller to reportError is only useful when we pass a string rather than an error object, which shouldn't happen here.

Right, though I meant it would be more useful to point to the extension code that called into our api.  But now that I think about it, if we failed to clone the callback argument, that's probably our error, and not an error with the extension.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

a year ago
mozreview-review
Comment on attachment 8954241 [details]
Bug 1441333: Part 4 - Add caller location to async API errors.

https://reviewboard.mozilla.org/r/223392/#review230496

r=me with the test added.
Attachment #8954241 - Flags: review?(tomica) → review+
Assignee

Comment 31

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9eb113f179f94145a575f602e359186d71b2b34
Bug 1441333: Part 1 - Add helper to retrieve closest stack frame with a given principal. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/61f204267373b1806950985a5b5abd4933f15009
Bug 1441333: Part 2 - Allow passing an error stack to reportError. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/c21a778bab3dac0c4c436f7473e80c4fa751d491
Bug 1441333: Part 3 - Add helper to create a JS error with a saved stack. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/47c2e679000ff75affdb8314a7ee83726411806e
Bug 1441333: Part 4 - Add caller location to async API errors. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf87433f501d755e6cde616a6ec77a4f37c69a00
Bug 1441333: Part 5 - Use proper async caller location in normalizeError. r=zombie

https://hg.mozilla.org/integration/mozilla-inbound/rev/930b44b7adc7e26e0b76cb46178489fc2e6704dc
Bug 1441333: Part 6 - Use caller location in error reports for StrongPromise errors. r=zombie
Assignee

Comment 32

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e11c53b5a51948e8afb845e2c1603cc2ecd8b9f
Bug 1441333: Follow-up: Fix OS-X failure due to extra console warning. r=bustage

Comment 34

a year ago
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
Assignee

Updated

a year ago
Flags: needinfo?(kmaglione+bmo) → qe-verify-

Comment 35

a year ago
mozreview-review
Comment on attachment 8954238 [details]
Bug 1441333: Part 1 - Add helper to retrieve closest stack frame with a given principal.

https://reviewboard.mozilla.org/r/223386/#review231970

You must ask a JS peer to review this code.

::: dom/base/ChromeUtils.h:171
(Diff revision 3)
>                                   const nsAString& id,
>                                   const nsAString& resourceURI,
>                                   ErrorResult& aRv);
> +
> +  static void
> +  GetCallerLocation(const GlobalObject& global, nsIPrincipal* principal,

aGlobal, aPrincipal

::: js/src/jsfriendapi.h:2999
(Diff revision 3)
> + * subsumed by the given |principals|. If there is no such frame, return nullptr.
> + *
> + * Do NOT pass a non-SavedFrame object here.
> + */
> +extern JS_FRIEND_API(JSObject*)
> +GetFirstSubsumedSavedFrame(JSContext* cx, JSPrincipals* principals, JS::HandleObject savedFrame, JS::SavedFrameSelfHosted selfHosted);

I cannot review this code.
Attachment #8954238 - Flags: review?(amarchesini)

Comment 36

a year ago
mozreview-review
Comment on attachment 8954238 [details]
Bug 1441333: Part 1 - Add helper to retrieve closest stack frame with a given principal.

https://reviewboard.mozilla.org/r/223386/#review231974

r+ for the DOM part.
Attachment #8954238 - Flags: review+

Comment 37

a year ago
mozreview-review
Comment on attachment 8954239 [details]
Bug 1441333: Part 2 - Allow passing an error stack to reportError.

https://reviewboard.mozilla.org/r/223388/#review231972

You need a r+ from a JS peer here as well.

::: js/xpconnect/src/XPCComponents.cpp:2090
(Diff revision 3)
> -    int32_t lineNo = 0;
> +    uint32_t lineNo = 0;
>  
>      if (!scripterr) {
> +        RootedObject stackObj(cx);
> +        if (stack.isObject()) {
> +            if (!JS::IsSavedFrame(&stack.toObject())) {

no {} in JS code style.

::: js/xpconnect/src/XPCComponents.cpp:2096
(Diff revision 3)
> +                return NS_ERROR_INVALID_ARG;
> +            }
> +
> +            stackObj = &stack.toObject();
> +
> +            if (GetSavedFrameLine(cx, stackObj, &lineNo) != SavedFrameResult::Ok) {

same here.
Attachment #8954239 - Flags: review?(amarchesini)

Comment 38

a year ago
mozreview-review
Comment on attachment 8954240 [details]
Bug 1441333: Part 3 - Add helper to create a JS error with a saved stack.

https://reviewboard.mozilla.org/r/223390/#review231976
Attachment #8954240 - Flags: review?(amarchesini) → review+
Assignee

Updated

a year ago
Blocks: 1448878

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.