1251529 - JavaScript allocation metadata should be collected by a builder class, not a callback

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

At the moment, the embedding registers a callback function with SpiderMonkey to provide allocation metadata to attach to new objects. Since we'll soon have string allocation metadata as well, we should replace this callback function with a builder object, and provide an appropriate base class.

Comment 1

[Jim Blandy :jimb]

Attachment: In object metadata world, rename "object metadata" to "allocation metadata" and "callback" to "builder". Exceptions: - AutoSetNewObjectMetadata and its related type are still exclusively about objects

The code that handles metadata now talks exclusively of "object metadata callbacks", but we want "allocation metadata builders". This is a purely mechanical patch that changes the names.

This does mean that we'll momentarily have a function pointer typedef named ...Builder, but I don't think that's entirely wrong. We'll replace it in the next patch anyway.

Comment 2

[Jim Blandy :jimb]

Attachment: Replace allocation metadata callback with a builder class.

Comment 3

[Jim Blandy :jimb]

Neither of these changes should have any visible effect on behavior, so there are no tests.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8723948 [details] [diff] [review]
Replace allocation metadata callback with a builder class.

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

::: js/src/builtin/TestingFunctions.cpp
@@ +1664,5 @@
>      AutoEnterOOMUnsafeRegion oomUnsafe;
>  
>      RootedObject obj(cx, NewBuiltinClassInstance<PlainObject>(cx));
>      if (!obj)
> +        oomUnsafe.crash("ShellAllocationMetadataBuilder::build");

If all the metadata callbacks are oom-unsafe in practice, maybe we should pass that in to ::build()?

Comment 5

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #4)
> If all the metadata callbacks are oom-unsafe in practice, maybe we should
> pass that in to ::build()?

Yeah, good suggestion. I didn't realize it, but JSCompartment::setNewObjectMetadata returns void.

Comment 6

[Jim Blandy :jimb]

Attachment: Pass AutoEnterOOMUnsafeRegion to allocation metadata builder methods.

Make metadata builder methods take an AutoEnterOOMUnsafeRegion, since they're not designed to report OOM conditions.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad5ff46b72e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc76f66bf11f
https://hg.mozilla.org/integration/mozilla-inbound/rev/12f6c52e4b4d

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d06527892bc

Comment 9

[Jim Blandy :jimb]

Attachment: Provide default constructor for js::AllocationMetadataBuilder, js::SavedStacks::MetadataBuilder. r=shu

This was required for the Clang builds.

The general reason seems to be explained here:
http://stackoverflow.com/questions/7411515/why-does-c-require-a-user-provided-default-constructor-to-default-construct-a

But since the class has a virtual method, it shouldn't be POD, so I don't really understand.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebc3275aa3d

Comment 11

[Jim Blandy :jimb]

Attachment: Provide default constructor for ShellAllocationMetadataBuilder. r=orange

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad5ff46b72e7
https://hg.mozilla.org/mozilla-central/rev/fc76f66bf11f
https://hg.mozilla.org/mozilla-central/rev/12f6c52e4b4d
https://hg.mozilla.org/mozilla-central/rev/0d06527892bc
https://hg.mozilla.org/mozilla-central/rev/4ebc3275aa3d