390175 - ActionMonkey: Add CustomMark() methods to SpiderMonkey types

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Attachment: v1

This is a pretty simple change.

Unfortunately I see no good way of testing this until we make the switch to MMgc allocation.

Comment 1

[Jason Orendorff [:jorendorff]]

Btw- this patch installs on top of the one in bug 389420.

Comment 2

[Jason Orendorff [:jorendorff]]

Note: The CustomMark() methods in this patch don't actually call MMgc::GC::SetMark() yet, because we're not using MMgc for allocation yet -- it would crash.

But when we switch to MMgc, the line in JS_CallTracer that reads:

    *flagp |= GCF_MARK;

will change to use SetMark(), so these objects will comply with what MMgc requires.

Comment 3

[Ed Lee :Mardak]

Comment on attachment 274488 [details] [diff] [review]
v1

>+protected:
>+    /* Helper function for implementing MMgc::GCFinalizedObject::Mark(). */
>+    void MarkAs(uint32 kind);
Should be "for implementing JSFinalizedGCThing::CustomMark. This could return bool to simplify switching things to use MMgc later.. see below.

>+bool
>+JSObject::CustomMark()
>+{
>+    MarkAs(JSTRACE_OBJECT);
>+    return true;
> }
We're returning true here even though we don't MMgc::GC::SetMark(). We could |return MarkAs()| and have MarkAs return false for now until we replace the GCF_MARK.

Comment 4

[Jason Orendorff [:jorendorff]]

I think the comment should say "MMgc::GCFinalizedObject::CustomMark()", because that's where the method is actually declared and documented. (There is no  JSFinalizedGCThing::CustomMark.)

>We're returning true here even though we don't MMgc::GC::SetMark().

As soon as we switch to MMgc for allocation, these methods will honor the full CustomMark() contract to the letter.

Right now, they honor the *spirit* of the contract--they each return true, indicating the object and everything reachable from it is marked.  Whether it's GC::SetMark()ed or GCF_MARKed is not the right way to look at it, I claim.

Comment 5

[Ed Lee :Mardak]

Comment on attachment 274488 [details] [diff] [review]
v1

(In reply to comment #4)
> I think the comment should say "MMgc::GCFinalizedObject::CustomMark()", because
> that's where the method is actually declared and documented. 
Even better.

> >We're returning true here even though we don't MMgc::GC::SetMark().
> Right now, they honor the *spirit* of the contract
Fair enough.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v2

Shake off bitrot; add one other trivial cleanup (say "ScanDelayedChildren(trc)" instead of "ScanDelayedChildren(rt->gcMarkingTracer)").

Comment 7

[Ed Lee :Mardak]

Comment on attachment 275168 [details] [diff] [review]
v2

>+     * Helper function for implementing
>+     * MMgc::GCFinalizedObject::CustomMark().
>+     */
>+    void MarkAs(uint32 kind);
r+ with ()s removed from comment.

Comment 8

[Jason Orendorff [:jorendorff]]

changeset 3743:  	08fddac94d35