1519154 - Devtools go into infinite loop after creating cyclic prototype chain and examining a specific object in it

Status: NEW

(DevTools :: Console, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1. Load any web page.
2. Evaluate:

     Location.prototype.__proto__ = { __proto__: location}
   

   in the console.

EXPECTED RESULTS: Get a cyclic prototype chain, but no infinite loop.

ACTUAL RESULTS: Infinite loop in devtools code. The tab no longer responds to clicks, can't be navigated via the URL bar, etc. The infinite loop is not even detectable by the slow script dialog, afaict (though maybe that's because it's system code and I have that turned off in my profile; I don't recall for sure).

I tried to experiment a bit more by putting that line in a web page (in a <script> tag) and then evaluating some things in the console. When I evaluate Location.prototype.__proto__ I get InternalError: too much recursion. When I evaluate Location.prototype I get the infinite loop.

There are proposals to make this particular cyclic prototype chain impossible (see https://github.com/heycam/webidl/pull/606), but I'm not sure whether this can happen in some other cases where cyclic prototype chains would still be allowed (any case that has a non-ordinary [[GetPrototypeOf]]).

Jim, do you have any idea who could look into this?

Comment 1

[:Harald Kirschner :digitarald]

ni Jim

Comment 2

[Jim Blandy :jimb]

I can look into this.

Comment 3

[Jim Blandy :jimb]

I believe that SpiderMonkey in general assumes that prototype chains are acyclic. There seems to be code to prevent the creation of such changes:

  /*
   * ES6 9.1.2 step 6 forbids generating cyclical prototype chains. But we
   * have to do this comparison on the observable WindowProxy, not on the
   * possibly-Window object we're setting the proto on.
   */
  RootedObject objMaybeWindowProxy(cx, ToWindowProxyIfWindow(obj));
  RootedObject obj2(cx, proto);
  while (obj2) {
    MOZ_ASSERT(!IsWindow(obj2));
    if (obj2 == objMaybeWindowProxy) {
      return result.fail(JSMSG_CANT_SET_PROTO_CYCLE);
    }

    bool isOrdinary;
    if (!GetPrototypeIfOrdinary(cx, obj2, &isOrdinary, &obj2)) {
      return false;
    }
    if (!isOrdinary) {
      break;
    }
  }

https://searchfox.org/mozilla-central/rev/b4ebbe90ae4d0468fe6232bb4ce90699738c8125/js/src/vm/JSObject.cpp#2885-2905

So the first question is, why isn't this triggering?

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the first question is, why isn't this triggering?

ES6 9.1.2 is the ordinary [[SetPrototypeOf]] internal method. It does cycle prevention in https://tc39.github.io/ecma262/#sec-ordinarysetprototypeof step 8.c, but only when all the objects involved have the ordinary [[GetPrototypeOf]] method.

Which means there is no cycle protection once objects have a non-ordinary [[GetPrototypeOf]], per spec. That includes the "location" object in Gecko at the moment (which also per spec has a non-ordinary [[GetPrototypeOf]], but also various other objects (WindowProxy, all Proxy instances, etc, etc). That's the break in the !isOrdinary case.

I haven't tried writing a similar testcase with a Proxy instance; maybe devtools knows to not trust those in situations like this...

There are spec proposals to add the cycle-checking for "location" somehow; that's where the testcase came from. There are also proposals to make Location.prototype an immutable prototype object in the sense of https://tc39.github.io/ecma262/#sec-immutable-prototype-exotic-objects which would also fix this specific testcase.

Comment 5

[Jim Blandy :jimb]

The loop is in that perennial favorite, _findSafeGetterValues in devtools/server/actors/object.js.

Comment 6

[Jim Blandy :jimb]

... and other places as well

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:nchevobbe, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.