Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(3 attachments, 5 obsolete attachments)

Assignee

Description

a year ago
LifoAllocScope is tricky to use, so I'd like to clean up users of LifoAlloc to avoid potential footguns.
Priority: -- → P3
Assignee

Updated

9 months ago
Assignee: nobody → tcampbell
Assignee

Updated

7 months ago
Attachment #8960673 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #8960674 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #8960675 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Attachment #8960677 - Attachment is obsolete: true
Assignee

Comment 5

7 months ago
Directly construct a LifoAllocScope on cx->tempLifoAlloc inside
RegExpObject::create. The data allocated temporarily does not outlive
this function. Also simplify callers as a result.
Assignee

Comment 6

7 months ago
Move LifoAllocScope out of BytecodeParser to avoid ordering issues that
may arise when LifoAllocScope is wrapped.

Depends on D9976
Assignee

Comment 7

7 months ago
Make off-thread and main-thread parsing more consistent by using the
JSContext tempLifoAlloc always. This also makes BytecodeCompiler APIs
more consistent.

Remove BytecodeCompiler::alloc which was obscuring lifetimes and
conflicts of allocator.

Depends on D9977
Assignee

Comment 8

7 months ago
To avoid risk of collisions while using tempLifoAlloc, this patch adds a
separate LifoAlloc for parse tasks.

Depends on D9978
Assignee

Comment 9

7 months ago
Why am I trying this?

LifoAllocScope represents a nestable set of temporary allocations. They should be as close to the lifetime we want as possible. It also gets confusing when we put LifoAllocScope inside of other types because we first risk leaking them to heap, and second hide the cleanup order.

Another class of problems is that multiple consumers use cx->tempLifoAlloc at the same time. The current usage is adhoc and hard to audit. In these patches, we change so that cx->tempLifoAlloc is only used as an argument to build a LifoAllocScope on the stack. This leads to something much easier to reason about and let's these temporary consumers remain isolated.

The final patch explores adding a parseLifoAlloc since the Parser uses marks directly and is hard to reason about. I'm not sure this is a good idea yet and it may instead make more sense to make the parser use LifoAllocScope.
Attachment #9020488 - Attachment is obsolete: true

Comment 10

7 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77e0c47e18d6
Cleanup LifoAlloc usage for RegExp r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/66ed28ca0a06
Cleanup LifoAlloc usage for BytecodeParser r=jorendorff
https://hg.mozilla.org/integration/autoland/rev/ce7098f9ff3f
Cleanup LifoAlloc usage for BytecodeCompiler r=jorendorff

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/77e0c47e18d6
https://hg.mozilla.org/mozilla-central/rev/66ed28ca0a06
https://hg.mozilla.org/mozilla-central/rev/ce7098f9ff3f
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
See Also: → 1504587
You need to log in before you can comment on or make changes to this bug.