1480835 - DEBUG-only hazard in AutoEntryScript constructor

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

This is a straightforward one. Here's the code:

AutoEntryScript::AutoEntryScript(JSObject* aObject,
                                 const char* aReason,
                                 bool aIsMainThread)
  : AutoEntryScript(xpc::NativeGlobal(aObject), aReason, aIsMainThread)
{
  MOZ_ASSERT(!js::IsCrossCompartmentWrapper(aObject));
}

Hazard is pasted below. But the problem is that xpc::NativeGlobal can GC, but we use aObject in the assertion in the body.

The easiest solution would be to delete the assertion.


Function '_ZN7mozilla3dom15AutoEntryScriptC4EP8JSObjectPKcb$void mozilla::dom::AutoEntryScript::AutoEntryScript(JSObject*, int8*, uint8)' has unrooted 'aObject' of type 'JSObject*' live across GC call '_ZN7mozilla3dom15AutoEntryScriptC1EP15nsIGlobalObjectPKcb$void mozilla::dom::AutoEntryScript::AutoEntryScript(nsIGlobalObject*, int8*, uint8)' at dom/script/ScriptSettings.cpp:672
    ScriptSettings.cpp:672: Call(1,2, __temp_1 := NativeGlobal(aObject*))
    ScriptSettings.cpp:672: Call(2,3, this*.AutoEntryScript(__temp_1*,aReason*,aIsMainThread*)) [[GC call]]
    ScriptSettings.cpp:674: Call(3,4, __temp_3 := IsCrossCompartmentWrapper(aObject*))
GC Function: _ZN7mozilla3dom15AutoEntryScriptC1EP15nsIGlobalObjectPKcb$void mozilla::dom::AutoEntryScript::AutoEntryScript(nsIGlobalObject*, int8*, uint8)
    _ZN7mozilla3dom15AutoEntryScriptC4EP15nsIGlobalObjectPKcb
    _ZN7mozilla3dom9AutoJSAPIC2EP15nsIGlobalObjectbNS0_24ScriptSettingsStackEntry4TypeE
    void mozilla::dom::AutoJSAPI::AutoJSAPI(nsIGlobalObject*, uint8, uint32)
    void mozilla::dom::AutoJSAPI::InitInternal(nsIGlobalObject*, JSObject*, JSContext*, uint8)
    uint8 JS_GetProperty(JSContext*, JS::Handle<JSObject*>, int8*, JS::MutableHandle<JS::Value>)
    uint8 JS_GetPropertyById(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)
    uint8 JS_ForwardGetPropertyTo(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)
    uint8 js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)
    IndirectCall: op

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Remove now-redundant assertion that is a rooting hazard

This code is from bug 1474541. In particular, see bug 1474541 comment 13.

Jan - I didn't look too closely, but it seems like what you mention in (2) is now done? The problem is that I'm fixing some problems in the hazard analysis related to losing call edges between constructor and destructor variants, and that reveals a hazard here. Is it ok to just kill off the assert now?

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8997500 [details] [diff] [review]
Remove now-redundant assertion that is a rooting hazard

Review of attachment 8997500 [details] [diff] [review]:
-----------------------------------------------------------------

Good find. This is fine, thanks!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/309fe3a601ac
Remove now-redundant assertion that is a rooting hazard, r=jandem

Comment 4

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/309fe3a601ac