Closed Bug 889543 Opened 10 years ago Closed 10 years ago

Use AutoCycleDetector for CloneMemory in vm/SelfHosting.cpp


(Core :: JavaScript Engine, defect)

Not set





(Reporter: terrence, Assigned: terrence)



(1 file)

Currently, json.cpp is using a custom built CycleDetector class. It could easily make use of the AutoCycleDetector in jscntxt.h, which provides several optimizations and automatic marking over what is used in json.cpp.
Does that also apply to CloneObject in SelfHosting.cpp?
Thanks, Till. It seems it could indeed by used there. I updated the json.cpp case while fixing a fuzz bug, so lets repurpose this bug to the CloneObject case.
Summary: Use AutoCycleDetector in json.cpp → Use AutoCycleDetector for CloneMemory in vm/SelfHosting.cpp
The existing CloneMemory mechanism reifies cycles by returning the already cloned object, instead of simply failing when it hits a cycle. This surprised me as I didn't think our current self-hosted primitives (or any primitives, for that matter) have a cycle. To check, I ran both test suites with an assertion instead of reification. Nothing failed, so I think it is probably fine to remove this capability and instead fail in the presence of cycles.
Assignee: general → terrence
Attachment #8389228 - Flags: review?(till)
Comment on attachment 8389228 [details] [diff] [review]

Review of attachment 8389228 [details] [diff] [review]:

Yeah, I guess we don't really need cycles in any self-hosted object graphs. However, the code should absolutely report an error in case there is a cycle instead of just silently failing: we've had too many instances of silent failure in self-hosted code already, making it pretty hard to debug at times.

Also, is the cycle checking expensive? If so, can we maybe only do it in debug builds? All self-hosted code should have good test coverage, and we're not dealing with dynamic objects, generally.

All of this is conditional on something I'm not 100% sure about: Intl not generating cyclic graphs. f?'ing waldo for verification of that.

(Also, sorry for the late review, vacation and all that ...)

::: js/src/vm/SelfHosting.cpp
@@ +1001,5 @@
> +    AutoCycleDetector detect(cx, selfHostedObject);
> +    if (!detect.init())
> +        return nullptr;
> +    if (detect.foundCycle())
> +        return nullptr;

As mentioned above, this absolutely must dump an error instead of failing silently.

@@ +1130,3 @@
>      }
> +
> +    gc::AutoSuppressGC suppress(cx);

Is this needed with the other changes in place? Seems to me like cloning should be GC-safe now, no?
Attachment #8389228 - Flags: review?(till)
Attachment #8389228 - Flags: review+
Attachment #8389228 - Flags: feedback?(jwalden+bmo)
Comment on attachment 8389228 [details] [diff] [review]

Review of attachment 8389228 [details] [diff] [review]:

Yeah, I don't think Intl does anything cycle-y, particularly not in self-hosting startup/init phases.
Attachment #8389228 - Flags: feedback?(jwalden+bmo) → feedback+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
The following changeset is now in Firefox Nightly:

> d29c145b465c Bug 889543 - Use AutoCycleDetector instead of CloneMemory; r=till

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1

Download Links:

>         Linux x86:
>      Linux x86_64:
> Linux x86_64 ASAN:
>               Mac:
>             Win32:
>             Win64:

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
You need to log in before you can comment on or make changes to this bug.