547814 - Workers: Need to call JS_MakeStringImmutable before passing strings across threads?

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Jason pointed this out, need to read https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Reference/JS_THREADSAFE#Sharing_strings_among_threads and do something about it.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Trunk and 1.9.2 patch

Comment 2

[Jason Orendorff [:jorendorff]]

Sorry for the false alarm. This is already safe. The patch isn't needed.

The string is never really accessible by two threads at once. It's created in one thread and stuck in an event, which is then posted to execute in another thread. Posting the event goes through a lock, so it's a clean hand-off. It's totally safe.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks!

Comment 4

[Daniel Veditz [:dveditz]]

Ben called and said there was a security problem after all. Hiding bug and reopening until it gets figured out.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

Trunk and 1.9.2 only.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 428462 [details] [diff] [review]
Patch

That's the right fix. Good catch.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 428462 [details] [diff] [review]
Patch

Super simple fix, we either need to take this or back the string sharing out before we ship the next 1.9.2 release.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Needs to bake on mozilla-central before we can take it on 1.9.2, doesn't it?

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Neither jst nor I think it's necessary in this case (it's too simple) but I am certainly ok waiting as long as we make this block 1.9.2.2. Just don't want to forget to land it before we ship!

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/68334cffbb6a

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 428462 [details] [diff] [review]
Patch

a1922=beltzner

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c15947bf8073

Comment 13

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

Do we need a test for this?

I don't see any way for QA to verify here.

Comment 14

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

(In reply to comment #13)
> Do we need a test for this?
> 
> I don't see any way for QA to verify here.

Ben, any comments?

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, not sure we can really test this... This code is hit for every single message sent to a worker now, so the code in question is definitely hit. We didn't crash in the tests before and we don't now so I'm not sure what to do. I'd just not worry about it.

Comment 16

[Daniel Veditz [:dveditz]]

Doesn't this problem apply to 1.9.1 as well?

What are the potential security risks here? If it's causing crashes that sounds like memory corruptions (sg:critical) but since you weren't able to actually cause crashes maybe not?

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

It's not an issue on 1.9.1 because the string sharing code only landed on trunk and 1.9.2.

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(We always copy on 1.9.1, which is safe).