Closed Bug 1480835 Opened Last year Closed Last year

DEBUG-only hazard in AutoEntryScript constructor

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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?
Attachment #8997500 - Flags: review?(jdemooij)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
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!
Attachment #8997500 - Flags: review?(jdemooij) → review+
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/309fe3a601ac
Remove now-redundant assertion that is a rooting hazard, r=jandem
https://hg.mozilla.org/mozilla-central/rev/309fe3a601ac
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.