570169 - Add assertions that gcthings do not leak across compartments

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Initially, the assertions will flunk all over the place, so I'll make them just print a warning instead of the usual fatal JS_ASSERT. We'll toughen them up later.

Comment 1

[Bernd]

I doubt that this blocks bug 563009

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 1, rearrange some code - v1

This is to avoid circular dependencies once part 2 adds a bunch of new assertion stuff in jscntxtinlines.h.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 2, add assertions - v1

They are not hard assertions. Right now they just print a line of noise to stdout:

*** Compartment mismatch 0x838fa70 vs. 0x8381898

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 3, cope with E4X and compiler-internal objects - v1

This isn't exactly pretty, but it's the shortest path, it won't hurt perf much, and we will clean it up later.

Comment 5

[Andreas Gal :gal]

Comment on attachment 451673 [details] [diff] [review]
Part 2, add assertions - v1

The capital ASSERT functions are a bit surprising and look like macros but we do that elsewhere too. Not vouching for completeness. XPConnect does reach into the engine's guts occasionally.

Comment 6

[Andreas Gal :gal]

Comment on attachment 451675 [details] [diff] [review]
Part 3, cope with E4X and compiler-internal objects - v1

E4X, have you no shame?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (From update of attachment 451675 [details] [diff] [review])
> E4X, have you no shame?

There's nothing shameful in the default compartment :-P.

Jason, do you prefer cls to clasp as canonical JSClass * variable name? We've used clasp for a long time. We had a contributor (first autoconf build system contributor), Chris Seawood, whose nick/username was cls, so I shied away from that. But it's a bit drop-the-vowels cyber-cruddy too.

/be

Comment 8

[Jason Orendorff [:jorendorff]]

Sorry, I'll fix it. I do prefer cls to clasp, as a personal foible, but the convention is really strong.

Comment 9

[Jason Orendorff [:jorendorff]]

This was briefly landed but I backed it out.

For one thing, I forgot that JS_NewArrayObject allows a NULL vector argument with a nonzero length argument. So it turned everything involving debug builds orange.

I'm running the updated patch through the try server now.

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bdea0d92907c

Comment 11

[Robert Sayre]

backed out

Comment 12

[Andreas Gal :gal]

Jason whats the status here?

Comment 13

[Chris Peterson [:cpeterson]]

jorendorff: are your r+'d patches (from 201) adding compartment leak asserts still relevant?

Comment 14

[Jason Orendorff [:jorendorff]]

No.