Closed Bug 889543 Opened 9 years ago Closed 9 years ago

Use AutoCycleDetector for CloneMemory in vm/SelfHosting.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(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
Status: NEW → ASSIGNED
Attachment #8389228 - Flags: review?(till)
Comment on attachment 8389228 [details] [diff] [review]
autocycledetect_for_clonememory-v0.diff

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]
autocycledetect_for_clonememory-v0.diff

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+
https://hg.mozilla.org/mozilla-central/rev/d29c145b465c
Status: ASSIGNED → RESOLVED
Closed: 9 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
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
You need to log in before you can comment on or make changes to this bug.