410323 - Assertion failure: !rt->gcRunning | ASSERTION: XPCWrappedNative::GetNewOrUsed called during GC

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: stack

browser only.
sensitive since bug 391851 is.

steps to reproduce:

1. load url in debug trunk build
   if you have access to the office network, I can give you access to a mac intel 
   box, if you have access to the colo, I can give you access to a winxp box.
2. shutdown
3. crash

###!!! ASSERTION: XPCWrappedNative::GetNewOrUsed called during GC: '!Scope->GetRuntime()->GetThreadRunningGC()', file /work/mozilla/builds/1.9.0/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 295
Assertion failure: !rt->gcRunning, at /work/mozilla/builds/1.9.0/mozilla/js/src/jsgc.c:1336
Trace/breakpoint trap

regressed sometime between approx midnight 12/27 PST and 1:30pm PST 12/30. I'll work on getting a better regression range unless someone else can point out the bug.

Comment 1

[Bob Clary [:bc] (inactive)]

This was caused by bug 399587. Do we explicitly mark regressions when one of the bugs is sensitive and the other isn't?

Comment 2

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Perhaps fixed by bug 402966?

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to comment #2)
> Perhaps fixed by bug 402966?

Nope. :-(

Comment 4

[Brendan Eich [:brendan]]

Giving to mrbkap per comment 1.

Jesse says this is making it impossible for him to test debug trunk -- that's bad.

/be

Comment 5

[Jesse Ruderman]

I hit this when I load https://mail.google.com/mail/ and also at other times.

Comment 6

[Jesse Ruderman]

It's silly for a regression this common to be security-sensitive, and I don't see anything in this bug that looks secret.

Comment 7

[Jesse Ruderman]

This is causing orange on a Windows Tinderbox:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1199406780.1199412571.21537.gz&fulltext=1

Comment 8

[Bob Clary [:bc] (inactive)]

fixed by backing out bug 399587 comment 16

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Not "fixed," wallpapered over. Reopening to find a real fix.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

The fix here is to not call into js_LookupProperty (which calls resolve hooks) in a function that is called from GC. Furthermore, I don't think this code wants resolve hooks to run: it only needs to change the sprop properties if the property already exists, not if the property has been newly created by such a hook. This fix makes us use the scope directly to see if the object has the property. Also note that you're not allowed to set a watch point on a non-native object (as enforced by JS_SetWatchPoint).

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 295406 [details] [diff] [review]
Fix

>+    JSScopeProperty *sprop, *sprop2;

Suggest wprop or longer (watched_sprop? nah, wprop) instead of sprop2.

>+        JS_LOCK_OBJ(cx, wp->object);
>+        scope = OBJ_SCOPE(wp->object);
>+        sprop2 = (scope->object == wp->object)
>+                 ? SCOPE_GET_PROPERTY(scope, sprop->id)
>+                 : NULL;
>+        JS_UNLOCK_SCOPE(cx, wp->object);

Oops -- need to pass scope to JS_UNLOCK_SCOPE.

>+        if (sprop2) {
>+            sprop = js_ChangeScopePropertyAttrs(cx, scope, sprop,
>+                                                0, sprop->attrs,
>+                                                sprop->getter,
>+                                                wp->setter);
>+            if (!sprop)
>+                ok = JS_FALSE;

Noting here that sprop2 is unused except as a boolean. Could avoid wprop in favor of a 'found' JSBool.

r/a=me with fixage.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 295427 [details] [diff] [review]
Updated to comments

found can cuddle with ok on the first declaration line. r+a=me with that!

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Checked in.

Comment 15

[Bob Clary [:bc] (inactive)]

v