361856 - Assertion failure: overwriting @ js_AddScopeProperty with certain watcher functions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

Steps to reproduce:

1. Load <http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_5/Regress/regress-361467.js;language=language;javascript>
2. Shutdown.

Occurs in 1.8.0.9, 1.8.1.1 and 1.9 windows/linux

>	js3250.dll!js_AddScopeProperty(JSContext * cx=0x04357f30, JSScope * scope=0x04367c88, long id=14677728, int (JSContext *, JSObject *, long, long *)* getter=0x031df5a0, int (JSContext *, JSObject *, long, long *)* setter=0x031df690, unsigned long slot=44, unsigned int attrs=1, unsigned int flags=0, int shortid=0)  Line 1128 + 0x1c bytes	C
 	js3250.dll!js_ChangeScopePropertyAttrs(JSContext * cx=0x04357f30, JSScope * scope=0x04367c88, JSScopeProperty * sprop=0x044c69e0, unsigned int attrs=1, unsigned int mask=1, int (JSContext *, JSObject *, long, long *)* getter=0x031df5a0, int (JSContext *, JSObject *, long, long *)* setter=0x031df690)  Line 1286 + 0x2c bytes	C
 	js3250.dll!js_ChangeNativePropertyAttrs(JSContext * cx=0x04357f30, JSObject * obj=0x043500e8, JSScopeProperty * sprop=0x044c69e0, unsigned int attrs=0, unsigned int mask=1, int (JSContext *, JSObject *, long, long *)* getter=0x031df5a0, int (JSContext *, JSObject *, long, long *)* setter=0x031df690)  Line 2907 + 0x21 bytes	C
 	js3250.dll!DropWatchPoint(JSContext * cx=0x04357f30, JSWatchPoint * wp=0x0466f288)  Line 272 + 0x2c bytes	C
 	js3250.dll!JS_ClearWatchPointsForObject(JSContext * cx=0x04357f30, JSObject * obj=0x043500e8)  Line 636 + 0x18 bytes	C
 	gklayout.dll!nsJSContext::ClearScope(void * aGlobalObj=0x043500e8, int aClearFromProtoChain=1)  Line 2945 + 0x11 bytes	C++
 	gklayout.dll!nsGlobalWindow::SetDocShell(nsIDocShell * aDocShell=0x00000000)  Line 1619	C++
 	docshell.dll!nsDocShell::Destroy()  Line 3484	C++
...

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

For a while, I was hitting this by going to a site with a watchpoint and closing the first window. I didn't have time to track down the problem then, but backing out the patch for bug 355339 made the assertion go away.

Comment 2

[shutdown]

shorter testcase.

$ cat assert-overwriting.txt
function test() {
  var obj = {};
  obj.watch("foo", function(){});
  delete obj.foo;
  obj = null;
  gc();
}
test();

$ dbg.obj/js assert-overwriting.txt
Assertion failure: overwriting, at jsscope.c:1128

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I'll look at this.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This fixes the assertion by removing the watchpoint when the property is deleted.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 249888 [details] [diff] [review]
Proposed fix

This is wrong, watchpoints survive deletion of a particular property. They are mappings from (obj, id) to handler function, not from (obj, property-currently-named-by-id).

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better offering

If the property went away already, there's nothing for us to do...

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With a comment

I forgot to mention, I think this patch also fixes a leak when js_ChangeNativePropertyAttrs fails.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 249950 [details] [diff] [review]
With a comment

Need updated patch to merge with thread safety fix.

Nit: order locals by first use, so ok before obj before prop. Might call obj pobj too.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated patch

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: One more fix

The last patch didn't propagate a js_ChangeNativePropertyAttrs failure.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 254669 [details] [diff] [review]
One more fix

Slight preference to avoid the goto outs and just test (ok && prop) { DROP; if (pobj == wp->object) {...} }.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 254773 [details] [diff] [review]
With that

>+        ok = js_LookupProperty(cx, wp->object, sprop->id, &pobj, &prop);
>+
>+        /*
>+         * If the property wasn't found on wp->object or didn't exist, then
>+         * someone else has dealt with this sprop, and we don't need to change
>+         * the property attributes.
>+         */
>+        if (ok && prop) {
>+            if (pobj == wp->object) {

At this point, you should be able to JS_ASSERT(OBJ_SCOPE(pobj)->object == pobj), and use pobj instead of wp->object below, *and* (to avoid nesting an object lock on pobj/wp->object) call js_ChangeScopePropertyAttrs directly, passing OBJ_SCOPE(pobj) instead of pobj/wp->object.

>+                sprop = js_ChangeNativePropertyAttrs(cx, wp->object, sprop,
>+                                                     0, sprop->attrs,
>+                                                     sprop->getter,
>+                                                     wp->setter);
>+                if (!sprop)
>+                    ok = JS_FALSE;

sprop is single-use -- could eliminate.

>+            }
>+            OBJ_DROP_PROPERTY(cx, pobj, prop);
>+        }
>     }
> 
>     js_RemoveRoot(cx->runtime, &wp->closure);
>     JS_free(cx, wp);
>-    return sprop != NULL;
>+    return ok;
> }
> 
> /*
>  * NB: js_MarkWatchPoints does not acquire cx->runtime->debuggerLock, since
>  * the debugger should never be racing with the GC (i.e., the debugger must
>  * respect the request model).
>  */
> void

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> sprop is single-use -- could eliminate.

sprop is used above all of this cruft, so it isn't single-use. I think that reusing it here is cleaner than wrapping the giant function call in an if statement.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: And again

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 255012 [details] [diff] [review]
And again

Good point, looks great.  Thanks,

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on trunk. Thanks for the great testcase, shutdown.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 255012 [details] [diff] [review]
And again

I believe that this assertion is making bc's life harder when he attempts to verify other watchpoint-related bugs.

Comment 19

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070226 windows/mac*/linux

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 255012 [details] [diff] [review]
And again

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 1.8 branch patch

This was pretty mechanical. I included the 'returning uninitialized ok' bug fix in this.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branches.

Comment 23

[Bob Clary [:bc] (inactive)]

verified fixed on 1.8.0, 1.8.1

Comment 24

[Bob Clary [:bc] (inactive)]

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