Closed Bug 1251529 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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.
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.
Attachment #8723947 - Flags: review?(nfitzgerald)
Neither of these changes should have any visible effect on behavior, so there are no tests.
Flags: in-testsuite-
Blocks: 1178505
Attachment #8723947 - Flags: review?(nfitzgerald) → review+
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()?
Attachment #8723948 - Flags: review?(nfitzgerald) → review+
(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.
Make metadata builder methods take an AutoEnterOOMUnsafeRegion, since they're not designed to report OOM conditions.
Attachment #8725070 - Flags: review?(nfitzgerald)
Attachment #8725070 - Flags: review?(nfitzgerald) → review+
Target Milestone: --- → mozilla48
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.
Attachment #8738015 - Flags: review+
Depends on: 1296243
You need to log in before you can comment on or make changes to this bug.