Closed
Bug 911333
Opened 11 years ago
Closed 11 years ago
Remove customTrace from mainthread bindings codegen
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
3.02 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Custom trace hooks are a huge footgun, and they are currently unused, so we should remove support for them from codegen. For instance, you have to be careful about barriers, and if you don't set a JSClass flag, then you'll end up permanently disabling incremental GC.
I'll also add a comment to the place where we will emit the null to warn people about it.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 801918 [details] [diff] [review]
remove it
try run: https://tbpl.mozilla.org/?tree=Try&rev=760c1d1023c0
Attachment #801918 -
Flags: review?(bzbarsky)
Comment 3•11 years ago
|
||
Comment on attachment 801918 [details] [diff] [review]
remove it
Remove the TRACE_HOOK_NAME variable too?
r=me
Attachment #801918 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 4•11 years ago
|
||
Ah, good catch. I was going to get rid of it, but then saw it was used another place, but then I deleted the other place it was used...
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4804e82856f
Comment 5•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 6•11 years ago
|
||
Reopening this, since we backed out. We'll try again once worker stuff is sane.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 7•11 years ago
|
||
I was thinking I could do a more limited version that removes the pref thing, so we're only doing customTrace on workers. I should also look at what workers are doing, as in a CC-fied setting using custom trace hooks is unorthodox.
Assignee | ||
Comment 8•11 years ago
|
||
Ah, it looks like bug 919457 separates out the notion of needing customTrace vs just being on a worker.
Summary: Remove customTrace from bindings codegen → Remove customTrace from mainthread bindings codegen
Updated•11 years ago
|
Target Milestone: mozilla26 → ---
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #801918 -
Attachment is obsolete: true
Attachment #818518 -
Flags: review?(khuey)
Comment on attachment 818518 [details] [diff] [review]
Eliminate bindings.conf setting for customTrace
Review of attachment 818518 [details] [diff] [review]:
-----------------------------------------------------------------
I need a custom trace hook for bug 919885, but I guess I could bundle it into the "customWrapperManagement" thing I added. What do you think?
Assignee | ||
Comment 11•11 years ago
|
||
Yeah, that seems reasonable enough to me. customWrapperManagement could just imply customFinalize. You'd probably want to add something to the comment in bindings.conf. It seems a little less dangerous when bundled up into something that is already pretty scary. ;)
Comment on attachment 818518 [details] [diff] [review]
Eliminate bindings.conf setting for customTrace
Review of attachment 818518 [details] [diff] [review]:
-----------------------------------------------------------------
r=me, even if I'm going to be grumpy about rebasing over it.
Attachment #818518 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 13•11 years ago
|
||
I can just wait until bug 919885 lands and figure out how to deal with it if it is going to be a pain.
Assignee | ||
Comment 14•11 years ago
|
||
This is kind of a footgun, but given that nobody seems to have ever used it aside from you, it can't be a very tempting one.
Shouldn't be hard. Should just need to add "or descriptor.customWrapperManagement" to "descriptor.nativeOwnership == 'worker'".
Comment 16•11 years ago
|
||
If we ever merge the Servo and Gecko codegen implementations, we're going to need to reintroduce this.
Assignee | ||
Comment 17•11 years ago
|
||
We could reintroduce it as "servoCustomTrace" or something. ;)
Assignee | ||
Comment 18•11 years ago
|
||
Comment 19•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•