600173 - atoms should live only in the default compartment

Status: RESOLVED DUPLICATE of bug 600593

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

See bug 558861 comment 76

Comment 1

[Igor Bukanov]

Nominating for 2.0 as this is a regression from bug 558861

Comment 2

[Igor Bukanov]

Attachment: v1

The patch copies the string if it does not come from the default compartment.

Comment 3

[Igor Bukanov]

Attachment: v2

In v1 I forgot to replace in flatLength() and flatLength() by just chars() and length() when js_AtomizeString copies the strings as now strings of any kind from a non-default-compartment are treated as temporary.

Comment 4

[Igor Bukanov]

Attachment: v3

fixing comments

Comment 5

[Gregor Wagner [:gwagner]]

Comment on attachment 479069 [details] [diff] [review]
v3


>+            if (needNewString) {
>                 SwitchToCompartment sc(cx, cx->runtime->defaultCompartment);
>-
>+                AutoUnlockDefaultCompartment unlock();
>+                jschar *chars = str->chars();
>                 if (flags & ATOM_NOCOPY) {
>-                    key = js_NewString(cx, str->flatChars(), str->flatLength());
>+                    key = js_NewString(cx, chars, length);
>                     if (!key)
>                         return NULL;

This means 2 threads could allocate a String in the same compartment at the same time.
That's not good!

Comment 6

[Igor Bukanov]

(In reply to comment #5)
> This means 2 threads could allocate a String in the same compartment at the
> same time.

It is OK as only one thread wins the race and the key would be looked up again - see relookupOrAdd call.

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to comment #6)
> (In reply to comment #5)
> > This means 2 threads could allocate a String in the same compartment at the
> > same time.
> 
> It is OK as only one thread wins the race and the key would be looked up again
> - see relookupOrAdd call.

No there could be a race condition in the allocation code if 2 threads try to get an object from the freelist at the same time. The freelist is now in the compartment and not in ThreadData.

Comment 9

[Andreas Gal :gal]

Why does this patch contain a bunch of unrelated changes to the int string code?

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > This means 2 threads could allocate a String in the same compartment at the
> > > same time.
> > 
> > It is OK as only one thread wins the race and the key would be looked up again
> > - see relookupOrAdd call.
> 
> No there could be a race condition in the allocation code if 2 threads try to
> get an object from the freelist at the same time. The freelist is now in the
> compartment and not in ThreadData.

Compartments are not single-threaded data structures?!

/be

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to comment #10)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (In reply to comment #5)
> > > > This means 2 threads could allocate a String in the same compartment at the
> > > > same time.
> > > 
> > > It is OK as only one thread wins the race and the key would be looked up again
> > > - see relookupOrAdd call.
> > 
> > No there could be a race condition in the allocation code if 2 threads try to
> > get an object from the freelist at the same time. The freelist is now in the
> > compartment and not in ThreadData.
> 
> Compartments are not single-threaded data structures?!
> 
> /be

Yes they are but in the case of allocating atoms we have to change to the defaultCompartment and that's the only place where we have to make sure that 2 threads can't enter the allocation path at the same time.

Comment 12

[Andreas Gal :gal]

Attachment: patch

Minimal patch. Just make a copy of the string if its in the wrong compartment.

NB: all compartments are single-threaded, _except_ the default compartment.

Comment 13

[Andreas Gal :gal]

Attachment: patch

Comment 15

[Andreas Gal :gal]

Attachment: patch

fix

Comment 16

[Gregor Wagner [:gwagner]]

Uh that doesn't work since we get a JSString from js_Atomize that's not heap allocated.

http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1285721694.1285721798.30677.gz&buildtime=1285721694&buildname=Rev3%20Fedora%2012%20tryserver%20debug%20test%20jsreftest&fulltext=1#err3

Comment 17

[Andreas Gal :gal]

Attachment: patch

This should work

Comment 18

[Igor Bukanov]

Comment on attachment 479259 [details] [diff] [review]
patch

>diff --git a/js/src/jsatom.cpp b/js/src/jsatom.cpp
>--- a/js/src/jsatom.cpp
>+++ b/js/src/jsatom.cpp
>@@ -454,25 +454,28 @@ JSAtom *
> js_AtomizeString(JSContext *cx, JSString *str, uintN flags)
> {
>     JS_ASSERT(!(flags & ~(ATOM_PINNED|ATOM_INTERNED|ATOM_TMPSTR|ATOM_NOCOPY)));
>     JS_ASSERT_IF(flags & ATOM_NOCOPY, flags & ATOM_TMPSTR);
> 
>     if (str->isAtomized())
>         return STRING_TO_ATOM(str);
> 
>+    str->ensureNotRope();
>+
>     size_t length = str->length();
>+    jschar *chars = str->chars();
>+
>     if (length == 1) {
>-        jschar c = str->chars()[0];
>+        jschar c = chars[0];
>         if (c < UNIT_STRING_LIMIT)
>             return STRING_TO_ATOM(JSString::unitString(c));
>     }
> 
>     if (length == 2) {
>-        jschar *chars = str->chars();
>         if (JSString::fitsInSmallChar(chars[0]) &&
>             JSString::fitsInSmallChar(chars[1])) {
>             return STRING_TO_ATOM(JSString::length2String(chars[0], chars[1]));
>         }
>     }
> 
>     /*
>      * Here we know that JSString::intStringTable covers only 256 (or at least
>@@ -491,16 +494,34 @@ js_AtomizeString(JSContext *cx, JSString
>                       (chars[1] - '0') * 10 +
>                       (chars[2] - '0');
> 
>             if (jsuint(i) < INT_STRING_LIMIT)
>                 return STRING_TO_ATOM(JSString::intString(i));
>         }
>     }
> 
>+    JS_ASSERT(!JSString::isStatic(str));
>+
>+    // If the string is not already in the default compartment, copy it there.
>+    if ((flags & ATOM_TMPSTR) ||
>+        (str->asCell()->compartment() != cx->runtime->defaultCompartment)) {
>+        SwitchToCompartment sc(cx, cx->runtime->defaultCompartment);
>+        AutoLockDefaultCompartment lock(cx);
>+
>+        // We conditionally unlock the lock if the default compartment is locked and we have
>+        // to GC.
>+        JSString *str2 = js_NewStringCopyN(cx, chars, length);

This will waste memory as the code ignores NOCOPY flag.

Comment 19

[Igor Bukanov]

Attachment: v5

This patch removes unrelated changes from js_AtomizeString.

Comment 20

[Igor Bukanov]

(In reply to comment #9)
> Why does this patch contain a bunch of unrelated changes to the int string
> code?

That was from another patch where I initially found and fixed the issue.

Comment 21

[Igor Bukanov]

(In reply to comment #11)
> Yes they are but in the case of allocating atoms we have to change to the
> defaultCompartment and that's the only place where we have to make sure that 2
> threads can't enter the allocation path at the same time.

Perhaps we should add separated atoms compartment? Or the goal is to ensure that only atoms would live in the default compartment effectively turning the default compartment into the default one.

Comment 22

[Andreas Gal :gal]

Comment on attachment 479379 [details] [diff] [review]
v5

This patch (again) introduces a race condition by entering the allocation path unlocked. v5 also seems to have lost the ropes fix. v4 was tested and works. I need this bug fixed for the compartments landing. I will spin off a bug that doesn't block b7/final to further optimize this issue w.r.t. to memory use. For now I need this to be correct for the compartment landing.

Comment 23

[Andreas Gal :gal]

Comment on attachment 479379 [details] [diff] [review]
v5

Moving v5 to a new bug.

Comment 24

[Igor Bukanov]

(In reply to comment #22)
> This patch (again) introduces a race condition by entering the allocation path
> unlocked.

I have missed that any allocations from the default compartment should be under the compartment lock. But that means that OOM reporting will happen under that lock as well. I guess js_ReportOutOfMemory should release that lock when executing the various user-supplied callbacks. OK, that should go to another bug.


> v5 also seems to have lost the ropes fix.

There is no need for it as str->chars() ensures that the string is not a rope, see comments in the code.

> v4 was tested and works.

That patch duplicates the string even if it is already in the table.

Comment 25

[Andreas Gal :gal]

Attachment: patch

Comment 26

[Brendan Eich [:brendan]]

If the default compartment is for atoms only, can we unify locking for allocation with existing atom state locking? Too many locks...

Also, holding locks across error reports is bad form. Elsewhere we release first, and this sometimes makes for a protocol where a callee is invoked with a lock held and if it succeeds, the lock is still held and invariants are maintained, but if it fails, the lock was released and an error has been reported. This is a valid pattern, we should use it if we can.

Abstracting over held-locks across hard-to-delimit critical sections and failure paths is almost always a design mistake.

/be

Comment 27

[Andreas Gal :gal]

(In reply to comment #26)
> If the default compartment is for atoms only, can we unify locking for
> allocation with existing atom state locking? Too many locks...

We already do that. Thats the lock we hold.

> 
> Also, holding locks across error reports is bad form. Elsewhere we release

We don't do that. We release the lock before reporting.

Comment 28

[Igor Bukanov]

(In reply to comment #27)

> We don't do that. We release the lock before reporting.

The OOM is still reported with the compartment lock held.

Comment 29

[Andreas Gal :gal]

Which one? The one in Atomize? We can add an AutoUnlock in that branch. Gregor want to do that?

Comment 30

[Igor Bukanov]

(In reply to comment #29)
> Which one? The one in Atomize? 

The one in RefillFinalizableFreeList, in str->undepend() and during allocation of the char array in js_NewStringCopyN.