1773324 - Avoid needing JSContext directly for parser to report errors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Ted Campbell [:tcampbell]]

In order to support parsing-to-stencil without a JSContext, we need to clean up error reporting. Fortunately this is already somewhat abstracted because helper-threads cannot allocate ErrorObjects but they checks around this still use the JSContext.

There will still be some non-trivial refactoring in ErrorReporting.cpp for these errors though.

Comment 1

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Separate use of JSContext for allocation into its own parameter r=arai

Comment 2

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Add ErrorReportMixin::getAllocator() r=arai

The type of the allocator will be varying separately from the type of the context.

Depends on D149996

Comment 3

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Introduce ErrorContext for handling error reports. r=arai

Depends on D149997

Comment 4

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Provide Parser ErrorContext as a constructor parameter. r=arai

This allows the Parser creator to provide the ErrorContext appropriate
for the situation (main thread or helper thread). For now, we just use
a flexible implementation.

I think the lifetime of the ErrorContext is the same as the Parser, so
putting the ErrorContext on the stack when the Parser is, too, should be
safe.

Depends on D150641

Comment 5

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Plumb ErrorContext parameter out to where main vs. helper thread is known r=arai

Depends on D150642

Comment 6

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Intro OffThreadErrorContext for off-thread compilation r=arai

Depends on D150643

Comment 7

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Support pending errors in OffThreadErrorContext r=arai

I could get rid of OffThreadErrorContext::cx_ by subclassing from
MallocProvider, but I don't think that would work since
OffThreadErrorContext has a vtable and MallocProvider does a
static_cast to get the client pointer.

OffThreadFrontendErrors moved to ErrorContext.h to avoid
recursive header inclusion.

Depends on D150644

Comment 8

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Pull OffThreadErrorContext to ParseTask member r=arai

This will allowe us to gather the errors after parsing without
needing a separate OffThreadFrontendErrors member.

Depends on D150645

Comment 9

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Report errors from ParseTask back to JSContext r=arai

Depends on D150646

Comment 10

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Add test covering reporting off-thread compilation errors r=arai

Depends on D150647

Comment 11

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Pass ErrorContext parameter to CompileModuleToStencil functions r=arai

Fixes jit-test. Probably needs coordination with ESMification efforts.

It looks like JS::CompileModule() would be called from the main thread, so
it should use GeneralErrorContext.

Depends on D150644

Comment 12

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Replace ErrorReporting allocator with ErrorAllocator r=arai

ErrorAllocator is an adapter that allows MallocProvider to delegate
to polymorphic Contexts. MallocProvider provides a well-known interface,
but doesn't support polymorphism, so we resort to templating and adapters
like this.

JS::CharsToNewUTF8CharsZ() needs a definition in CharacterEncoding.cpp,
so for now I explicitly instantiate it for ErrorAllocator<JSContext>.
A better solution would be to move the definition to a header, but that's
a bigger task.

Depends on D150648

Comment 13

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Remove ErrorReporting dependency on JSContext for GC-safe callback r=arai

ReportErrorVA(), ReportErrorNumberVA(), and ReportErrorNumberUCArray(), and
ReportErrorNumberUTF8Array() use JSContext for rooting, so they will have to be
called from the main thread. The only reason to pass ErrorContext to them would
be for consistency with the other error reporting functions.

Depends on D151179

Comment 14

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Remove template from ErrorAllocator, add ErrorContext::getAllocator(), rename OffThreadErrorContext::linkWithJSContext() r=arai

ErrorAllocator was only ever used with ErrorContext, so no need for templating.

linkWithJSContext() better describes what it is doing, since JSContext needs its
errors_ field filled in, at least until we can replace it with a different
allocator for functions that are called during compilation.

getAllocator() is a convenience so we don't have to add a separate allocator
parameter everywhere we pass an ErrorContext and also need an allocator.

Depends on D151608

Comment 15

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Access ErrorAllocator via ErrorContext accessor r=arai

This removes a lot of plumbing allocators through function parameters but still
maintains the ErrorContext and allocator as separate concepts.

At this point, there is no need for the type or instances of ErrorContext and
ErrorAllocator to vary independently.

Depends on D151609

Comment 16

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Fix typo r=arai

Depends on D151610

Comment 17

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Move implementations for ErrorContext to their own file r=arai

Depends on D151611

Comment 18

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Rename MainThreadErrorContext r=arai

"GenericErrorContext" hasn't been an appropriate name since 6b27a74292dc.

Depends on D151612

Comment 19

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Simplify reporting errors to ErrorContext r=arai

At the cost of moving the filled-in CompileError into the allocated space for OffThreadErrorContext.

Depends on D151667

Comment 20

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Add ErrorContext::hadErrors() and organize methods r=arai

hadErrors() is more descriptive than errors().empty().

Access to the pending errors list is pushed down to OffThreadErrorContext,
because that's the only context where it is used.

Depends on D151668

Comment 21

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Rename onAllocationOverflow() to match naming of onOutOfMemory() r=arai

Depends on D151669

Comment 22

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Clean up unneeded code r=arai

Depends on D151670

Comment 23

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Support off-thread delazification using ErrorContext r=arai

Moving recordErrors in DelazifyTask::Create() to after the allocation of the
task allows us to use the ErrorContext in the task to store the errors instead
of having to move them into the task later, and we only miss the allocation of
the task itself (which wasn't recorded in the errors anyway).

Depends on D151671

Comment 24

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Remove dead JSContext methods for helper threads r=arai

addPendingCompileError() is replaced by OffThreadErrorContext::reportError().

Depends on D151878

Comment 25

[Bryan Thrall [:bthrall]]

Attachment: Bug 1773324 - Pass ErrorContext down through Smoosh functions r=arai

Depends on D152191

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5369fa38a413
Separate use of JSContext for allocation into its own parameter r=arai
https://hg.mozilla.org/integration/autoland/rev/b31863ca872e
Add ErrorReportMixin::getAllocator() r=arai
https://hg.mozilla.org/integration/autoland/rev/61e92a33c07d
Introduce ErrorContext for handling error reports. r=arai
https://hg.mozilla.org/integration/autoland/rev/b6f70c94d870
Provide Parser ErrorContext as a constructor parameter. r=arai
https://hg.mozilla.org/integration/autoland/rev/cf0dada16695
Plumb ErrorContext parameter out to where main vs. helper thread is known r=arai
https://hg.mozilla.org/integration/autoland/rev/c3d1f8ea4873
Intro OffThreadErrorContext for off-thread compilation r=arai
https://hg.mozilla.org/integration/autoland/rev/4bc7da9d66df
Pass ErrorContext parameter to CompileModuleToStencil functions r=arai
https://hg.mozilla.org/integration/autoland/rev/86c6381232c7
Support pending errors in OffThreadErrorContext r=arai
https://hg.mozilla.org/integration/autoland/rev/bb106324e318
Pull OffThreadErrorContext to ParseTask member r=arai
https://hg.mozilla.org/integration/autoland/rev/aef2d3304fc6
Report errors from ParseTask back to JSContext r=arai
https://hg.mozilla.org/integration/autoland/rev/6c044f146772
Add test covering reporting off-thread compilation errors r=arai
https://hg.mozilla.org/integration/autoland/rev/ed424d92b1c2
Replace ErrorReporting allocator with ErrorAllocator r=arai
https://hg.mozilla.org/integration/autoland/rev/d4df7e3387c4
Remove ErrorReporting dependency on JSContext for GC-safe callback r=arai
https://hg.mozilla.org/integration/autoland/rev/e31782ad0907
Remove template from ErrorAllocator, add ErrorContext::getAllocator(), rename OffThreadErrorContext::linkWithJSContext() r=arai
https://hg.mozilla.org/integration/autoland/rev/e2184ca5ef0f
Access ErrorAllocator via ErrorContext accessor r=arai
https://hg.mozilla.org/integration/autoland/rev/99db76fde555
Fix typo r=arai
https://hg.mozilla.org/integration/autoland/rev/2588027facdf
Move implementations for ErrorContext to their own file r=arai
https://hg.mozilla.org/integration/autoland/rev/0b9187cc4be4
Rename MainThreadErrorContext r=arai
https://hg.mozilla.org/integration/autoland/rev/0cb5b56b74fb
Simplify reporting errors to ErrorContext r=arai
https://hg.mozilla.org/integration/autoland/rev/f3c25d3a623c
Add ErrorContext::hadErrors() and organize methods r=arai
https://hg.mozilla.org/integration/autoland/rev/10d24e14a707
Rename onAllocationOverflow() to match naming of onOutOfMemory() r=arai
https://hg.mozilla.org/integration/autoland/rev/67dcf42b40bd
Clean up unneeded code r=arai
https://hg.mozilla.org/integration/autoland/rev/01aa3be551d3
Support off-thread delazification using ErrorContext r=arai
https://hg.mozilla.org/integration/autoland/rev/da28877d8368
Remove dead JSContext methods for helper threads r=arai
https://hg.mozilla.org/integration/autoland/rev/dea62af6ee07
Pass ErrorContext down through Smoosh functions r=arai

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5369fa38a413
https://hg.mozilla.org/mozilla-central/rev/b31863ca872e
https://hg.mozilla.org/mozilla-central/rev/61e92a33c07d
https://hg.mozilla.org/mozilla-central/rev/b6f70c94d870
https://hg.mozilla.org/mozilla-central/rev/cf0dada16695
https://hg.mozilla.org/mozilla-central/rev/c3d1f8ea4873
https://hg.mozilla.org/mozilla-central/rev/4bc7da9d66df
https://hg.mozilla.org/mozilla-central/rev/86c6381232c7
https://hg.mozilla.org/mozilla-central/rev/bb106324e318
https://hg.mozilla.org/mozilla-central/rev/aef2d3304fc6
https://hg.mozilla.org/mozilla-central/rev/6c044f146772
https://hg.mozilla.org/mozilla-central/rev/ed424d92b1c2
https://hg.mozilla.org/mozilla-central/rev/d4df7e3387c4
https://hg.mozilla.org/mozilla-central/rev/e31782ad0907
https://hg.mozilla.org/mozilla-central/rev/e2184ca5ef0f
https://hg.mozilla.org/mozilla-central/rev/99db76fde555
https://hg.mozilla.org/mozilla-central/rev/2588027facdf
https://hg.mozilla.org/mozilla-central/rev/0b9187cc4be4
https://hg.mozilla.org/mozilla-central/rev/0cb5b56b74fb
https://hg.mozilla.org/mozilla-central/rev/f3c25d3a623c
https://hg.mozilla.org/mozilla-central/rev/10d24e14a707
https://hg.mozilla.org/mozilla-central/rev/67dcf42b40bd
https://hg.mozilla.org/mozilla-central/rev/01aa3be551d3
https://hg.mozilla.org/mozilla-central/rev/da28877d8368
https://hg.mozilla.org/mozilla-central/rev/dea62af6ee07