1255817 - Make AutoJSAPI always take ownership of error reporting

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I audited all our callsites and will post the results in a bit once I file the bugs tracking things that need fixing.  But once those things are fixed, we can do this.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Audit results, in case reviewers want to see them

Note that line numbers might be slightly off because of other code changes, of course.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that things that are particularly interesting in the audit are marked with "XXX" comments in that attachment.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make AutoJSAPI always take ownership of error reporting

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Get rid of AutoJSAPI::OwnsErrorReporting and AutoJSAPI::TakeOwnershipOfErrorReporting

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Remove the now-unreachable JS_ReportPendingException call in nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make XPCJSRuntime use MozCrashErrorReporter by default, now that there should be no stray JS_ReportPendingException calls

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Remove the now-unused xpc::SystemErrorReporter

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Fix up some comments in CallbackObject that refer to things that no longer exist

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make AutoJSAPI always take ownership of error reporting

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8729790 [details] [diff] [review]
part 5.  Remove the now-unused xpc::SystemErrorReporter

Review of attachment 8729790 [details] [diff] [review]:
-----------------------------------------------------------------

\o/ \o/ \o/

Comment 11

[Boris Zbarsky [:bzbarsky]]

So I realized there are three more sources of AutoJSAPI that I had not audited yet: AutoSafeJSContext, AutoJSContext, ThreadSafeAutoJSContext (these last two only on mainthread when there is nothing on the JSContext stack).  I think my plan is as follows:

1)  Audit AutoSafeJSContext uses.  I will attach that audit here.
2)  Get rid of ThreadSafeAutoJSContext in favor of passing JSContext* directly.
3)  Assume that any AutoJSContext consumer who is running when the JSContext stack is empty really does want
    to report the error, since there is nowhere else for it to go.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Audit results for AutoSafeJSContext

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96449936b41a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a45a6cde6558
https://hg.mozilla.org/integration/mozilla-inbound/rev/870cb629e575
https://hg.mozilla.org/integration/mozilla-inbound/rev/564aabb49527
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44267ae61b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d07dab8a233

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96449936b41a
https://hg.mozilla.org/mozilla-central/rev/a45a6cde6558
https://hg.mozilla.org/mozilla-central/rev/870cb629e575
https://hg.mozilla.org/mozilla-central/rev/564aabb49527
https://hg.mozilla.org/mozilla-central/rev/b44267ae61b9
https://hg.mozilla.org/mozilla-central/rev/1d07dab8a233