search

mceSystems / node-jsc

A node.js port to the JavaScriptCore engine and iOS
Other
221 stars 16 forks source link

Initial GC stabilizing patch #15

Closed Constellation closed 3 years ago

Constellation commented 5 years ago

I found that node-jsc crashes when running npm command. After investigating, I found that some of the objects' fields are not marked correctly. Since these objects are allocated in the GC heap, we need to mark the fields in visitChildren function.

This series of patches are the first attempt of fixing these GC issues. Yeah, still some issues remain. For example interceptors' JSValue should be WriteBarrier and marked correctly. But this one attempts to fix the major issues.

MCE-KobyBo commented 5 years ago

Thanks again for taking the time to do this! It's great to have someone who knows WebKit helping with node-jsc :) A few notes\questions before merging this:

  1. I've faced these kind of issues before where I've missed marking a member I've added and went through the code to find it (I obviously missed a few). Did you also just go through the code or is there a convenient way in WebKit to track GC problems like these?
  2. Regarding JSCStackFrame, I originally wanted to keep it a simple, stack allocated, object and avoid the GC marking (hence the "Note that this is not a heap allocated, garbage collected, JSCell. It must be stack allocated, as it doens't use any write barriers and rely on the GC to see the stored JSC object pointers on the stack." comment). But, as I'm using WTF::Vector to hold them, it's not safe and you're right, It's better to just mark everything.
  3. Regarding appendUnbarriered, the reason I've used it is due to compilation errors when passing objects of my own classes (due to the template WriteBarrierBase). Since I saw SlotVisitor::append is basically "appendUnbarriered(weak.get());", I just called it directly. Is it not the same? in the meantime I'll test it on MSVC\iOS before merging to check for the compilation errors.
Constellation commented 5 years ago

I've faced these kind of issues before where I've missed marking a member I've added and went through the code to find it (I obviously missed a few). Did you also just go through the code or is there a convenient way in WebKit to track GC problems like these?

Read through the code to find it. I think the best way is adding visitor.append when we add a new field!

Regarding JSCStackFrame, I originally wanted to keep it a simple, stack allocated, object and avoid the GC marking (hence the "Note that this is not a heap allocated, garbage collected, JSCell. It must be stack allocated, as it doens't use any write barriers and rely on the GC to see the stored JSC object pointers on the stack." comment). But, as I'm using WTF::Vector to hold them, it's not safe and you're right, It's better to just mark everything.

Yeah, so, basically, holding JSValue in a class field is a bug. We should add WriteBarrier<> and marking function in GC-managed cell or use Strong<>. Holding raw GC-managed pointer is super tricky and rare case. When we really want to do that, I recommend adding descriptive comments about why it is safe ;)

Regarding appendUnbarriered, the reason I've used it is due to compilation errors when passing objects of my own classes (due to the template WriteBarrierBase). Since I saw SlotVisitor::append is basically "appendUnbarriered(weak.get());", I just called it directly. Is it not the same? in the meantime I'll test it on MSVC\iOS before merging to check for the compilation errors.

::append only takes WriteBarrier. It finds bugs when we missed using WriteBarrier. appendUnbarriered is used only when we have a super tricky class that can’t have WriteBarrier. So, basically, we should not use this. It is somewhat an escape hatch for a super tricky case. If we can use append, using append is always better :)

Constellation commented 5 years ago

Any update on this?