Closed Bug 361856 Opened 13 years ago Closed 13 years ago

Assertion failure: overwriting @ js_AddScopeProperty with certain watcher functions

Categories

(Core :: JavaScript Engine, defect, P2)

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: bc, Assigned: mrbkap)

References

()

Details

(Keywords: crash, verified1.8.0.12, verified1.8.1.4)

Attachments

(2 files, 6 obsolete files)

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++
...
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.
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
I'll look at this.
Assignee: general → mrbkap
Attached patch Proposed fix (obsolete) — Splinter Review
This fixes the assertion by removing the watchpoint when the property is deleted.
Attachment #249888 - Flags: review?(brendan)
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
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
Attachment #249888 - Flags: review?(brendan) → review-
Attached patch Better offering (obsolete) — Splinter Review
If the property went away already, there's nothing for us to do...
Attachment #249888 - Attachment is obsolete: true
Attachment #249949 - Flags: review?(brendan)
Attached patch With a comment (obsolete) — Splinter Review
I forgot to mention, I think this patch also fixes a leak when js_ChangeNativePropertyAttrs fails.
Attachment #249949 - Attachment is obsolete: true
Attachment #249950 - Flags: review?(brendan)
Attachment #249949 - Flags: review?(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
Attachment #249950 - Flags: review?(brendan)
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #249950 - Attachment is obsolete: true
Attachment #254615 - Flags: review?(brendan)
Attached patch One more fix (obsolete) — Splinter Review
The last patch didn't propagate a js_ChangeNativePropertyAttrs failure.
Attachment #254615 - Attachment is obsolete: true
Attachment #254669 - Flags: review?(brendan)
Attachment #254615 - Flags: review?(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
Attached patch With that (obsolete) — Splinter Review
Attachment #254669 - Attachment is obsolete: true
Attachment #254773 - Flags: review?(brendan)
Attachment #254669 - Flags: review?(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
(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.
Attached patch And againSplinter Review
Attachment #254773 - Attachment is obsolete: true
Attachment #255012 - Flags: review?(brendan)
Attachment #254773 - Flags: review?(brendan)
Comment on attachment 255012 [details] [diff] [review]
And again

Good point, looks great.  Thanks,

/be
Attachment #255012 - Flags: review?(brendan) → review+
Fixed on trunk. Thanks for the great testcase, shutdown.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Attachment #255012 - Flags: approval1.8.1.3?
Attachment #255012 - Flags: approval1.8.0.11?
Flags: in-testsuite+
verified fixed 1.9.0 20070226 windows/mac*/linux
Status: RESOLVED → VERIFIED
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
Attachment #255012 - Flags: approval1.8.1.4?
Attachment #255012 - Flags: approval1.8.1.4+
Attachment #255012 - Flags: approval1.8.0.12?
Attachment #255012 - Flags: approval1.8.0.12+
Attached patch 1.8 branch patchSplinter Review
This was pretty mechanical. I included the 'returning uninitialized ok' bug fix in this.
Fixed on the 1.8 branches.
verified fixed on 1.8.0, 1.8.1
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-361856.js,v  <--  regress-361856.js
initial revision: 1.1
You need to log in before you can comment on or make changes to this bug.