1046529 - Not all static atoms end up in gStaticAtomTable

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Nicholas Nethercote [inactive]]

I found what I think is a bug with the static atoms table, and bz agrees.

When adding a new static atom, RegisterStaticAtoms() looks in the normal atoms table first. There are two cases.

(a) If the atom is already there, it is marked as permanent.

(b) Otherwise, RegisterStaticAtoms() installs (permanently) the new static atom in the normal atoms table, and also puts it in the static atoms table.

The bug is that in case (a), the "and also puts it in the static atoms table" step is missing. Which means that subsequent calls to NS_GetStaticAtom() with the corresponding string will fail.

It looks like the "and also puts it in the static atoms table" code should be moved below the if/else, so it always runs.

hsivonen, does this diagnosis and proposed fix sound correct to you?

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: List of static atoms cases

Here's a list of all the inserted static atoms, and whether it falls into case (a) or case (b). There are 1127 in case (a), which is the buggy one, and 2604 in case (b), which is the ok one.

Comment 2

[Nicholas Nethercote [inactive]]

Hmm... here's case (a):

>    if (he->mAtom) {
>      fprintf(stderr, "a\n");
>      if (!he->mAtom->IsPermanent()) {
>        // We wanted to create a static atom but there is already one there. So
>        // convert it to a non-refcounting permanent atom.
>        fprintf(stderr, "promote\n");
>        PromoteToPermanent(he->mAtom);
>      }
> 
>      *aAtoms[i].mAtom = he->mAtom;

The "We wanted to..." sub-case doesn't occur when I start Firefox. I think that explains why things aren't horribly broken -- most of those 1127 case (a) occurrences in the attachment must be re-insertions of static atoms.

Still, the possibility that a static atom doesn't get inserted in the static atoms table remains, and seems worth fixing.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Split and simplify AtomTableKey's constructors

Some preliminary clean-up: each of AtomTableKey's constructors has two modes:
one where |aHash| is an inparam, and one where it's an outparam. This is really
confusing, so this patch splits each one in two. This allows UpdateHashKey() to
be inline, which makes everything clearer.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Ensure static atoms always end up in gStaticAtomTable

With this change, atoms can be inserted multiple times into gStaticAtomTable,
but that's ok.

Comment 5

[Nicholas Nethercote [inactive]]

Something that still leaves me uncomfortable is that "static atom" has two, subtly different meanings.

- The first meaning is the same as "permanent". I.e. IsStaticAtom() is the same as IsPermanent(). Both NS_NewPermanentAtom() and RegisterStaticAtoms() create static atoms with this meaning.

- The second meaning is narrower, and is implied by NS_GetStaticAtom(). Atoms registered with RegisterStaticAtoms() qualify, but those registered with NS_NewPermanentAtom() do not.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465203 [details] [diff] [review]
(part 1) - Split and simplify AtomTableKey's constructors

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8465204 [details] [diff] [review]
(part 2) - Ensure static atoms always end up in gStaticAtomTable

Seems reasonable.

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Nicholas Nethercote [:njn] from comment #0)
> hsivonen, does this diagnosis and proposed fix sound correct to you?

Yes, but I haven't worked with the static atom table on this level of detail. IIRC, sicking has.

Comment 9

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5903b0f685fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e21fe84c02

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5903b0f685fc
https://hg.mozilla.org/mozilla-central/rev/d9e21fe84c02