614459 - Privatize fields in JSString

Status: RESOLVED DUPLICATE of bug 613457

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

JSString's fields are currently public.  This is because we need to statically initialize various strings.  It also means that JSString doesn't inherit from js::gc::Cell.

Luke suggested that we could introduce a StringStaticInitializer class that has public fields that mirror JSString's (for flat atomized strings, which are the only kind that are statically initialized), use that for the statically initialized strings, and then cast them to JSString.  We'd need static assertions to ensure the two types have identical layouts.

Then JSString's fields could become private, and we could strengthen the safety of field accesses, which is good because JSString's fields are very complicated (eg. there are multiple unions) due to all the different kinds of string.  JSString could also inherit from js::gc::Cell and some special-casing could be removed (including Cell::asCell(), IIUC).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch (agains TM 58010:9123f97f059c)

This patch:

- Introduces JSStaticString, which has identical layout to (flat, short)
  JSString, and converts all the static strings to use the new type.

- Casts JSStaticString to JSString where necessary.

- Marks the fields in JSString as private.  Some new member functions (eg.
  externalType(), charsOffset()) were required for this.

- Makes JSString a sub-class of js::gc::Cell.  This allowed Cell::asCell()
  to be removed.  A few |thing->asCell()| occurrences -- ones compared
  against FreeCell pointers -- had to become |static_cast<Cell *>(thing)|.

- In js_IntToString(), does the conversion into a temporary buffer and then
  copies the result into the short string.  This allowed some of the weirder
  functions -- getINlineStorageBeforeInit(), initAtOffsetInBuffer() -- to be
  removed.  Instruction counts are changed only infinitesimally.

- Changes JSShortString to be a sub-class of JSString (with extra storage)
  rather than containing a JSString.  I did this because when it contained a
  JSString *and* both JSString and JSShortString were subclasses of
  js::gc::Cell, its size increased by a word -- I'm not sure why -- and the
  static assertions about layout failed.  Making JSShortString a sub-class
  makes it more like JSExternalString which isn't a bad thing.

- Adds JSStringLayoutChecks, a class for holding all the static asserts
  about the layouts of JSString, JSShortString, JSStaticString.  It's a
  friend of JSString and JSShortString so it can access their private 
  fields.
  
The patch will need a bit of updating after bug 613457 lands.