475971 - js_CheckRedeclaration does not always unlock object on failures

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

The contract for js_CheckRedeclaration is to always call OBJ_DROP_PROPERTY on failures. But this does not happen when the caller passes non-null objp/propp. For objects shared between threads this leads to unbalanced js_(Lock/Unlock)Object. The example below demonstrates this. Currently it hangs when run in a thread-safe shell.

Note this probably is not reproducible in the browser as even with thread workers no objects should be shared between threads.  

function x() { return 1; };

// g must run sufficiently long to ensure that the global scope is accessed                                                                                        
// from the parallel threads.                                                                                                                                      
function g()
{
    var sum = 0;
    try {
        for (var i = 0; i != 10000; ++i) {
            sum += x();
        }
    } catch (e) { }
}

scatter([g, g]);

try {
    eval("const x = 1");
} catch (e) { }

scatter([g, g]);

print("Done");

Comment 1

[Igor Bukanov]

The fix for the bug 476049 has landed in TM and it also fixes this issue.

Comment 2

[Igor Bukanov]

Nominating for 1.9.0

Comment 3

[Igor Bukanov]

The fix for the bug 476049 has landed in mozilla-central and it also fixes this issue.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: js1_8/extensions/regress-475971.js

Comment 5

[Igor Bukanov]

fixed for 1.9.0 as the fix for bug 476049 is landed on this branch

Comment 6

[Igor Bukanov]

Regarding sg:critical - this bug itself is not critical. The reason that the patch for bug 476049 included the fix was that it was easy to make the patch there that would fix this bug rather than not.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.9 as bug 476049 has been verified for 1.9.0.9. 

Bob, I don't suppose that this test could be added to the 1.9.0 test runs?

Comment 8

[Bob Clary [:bc] (inactive)]

Al, it has been although it is not checked in since it is sensitive. No need to verify js bugs that have in-testsuite+ marked since I'll do them.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

All right. I'm just trying to make sure 1.9.0.9 bugs are verified by today since we make builds on Monday and I'd hate to find out that we didn't fix something AFTER we have official builds.

Comment 10

[Bob Clary [:bc] (inactive)]

k.

Comment 11

[Marc Bejarano]

bob: test checked in?

Comment 12

[Bob Clary [:bc] (inactive)]

no, not yet.

Comment 13

[Marc Bejarano]

shouldn't this be "in-testsuite -" then?

Comment 14

[Bob Clary [:bc] (inactive)]

No. It has a test attached to the bug and that will be checked in shortly.

Comment 15

[Marc Bejarano]

hi bob.  did it get checked in?

Comment 16

[Bob Clary [:bc] (inactive)]

js1_8/extensions/regress-475971.js	
http://hg.mozilla.org/tracemonkey/rev/b399794774c4

Comment 17

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8/extensions/regress-475971.js,v  <--  regress-475971.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

v no crashes with 1.9.1, 1.9.2, 1.9.3