Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: free event targets properties by gc mark. #929

Merged
merged 2 commits into from
Dec 6, 2021
Merged

fix: free event targets properties by gc mark. #929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1964cc8
fix: free event targets properties by gc mark.
andycall Nov 30, 2021
00d3d0f
fix: use Object.defineProperty to define object's properties.
andycall Nov 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions bridge/bindings/qjs/bom/window.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ PROP_GETTER(Window, __location__)(QjsContext* ctx, JSValue this_val, int argc, J
auto* window = static_cast<WindowInstance*>(JS_GetOpaque(this_val, 1));
if (window == nullptr)
return JS_UNDEFINED;
return JS_DupValue(ctx, window->m_location->jsObject);
return JS_DupValue(ctx, window->m_location.value());
}
PROP_SETTER(Window, __location__)(QjsContext* ctx, JSValue this_val, int argc, JSValue* argv) {
return JS_NULL;
Expand Down Expand Up @@ -183,8 +183,6 @@ void WindowInstance::gcMark(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func)
EventTargetInstance::gcMark(rt, val, mark_func);

// Should check object is already inited before gc mark.
if (JS_IsObject(m_location->jsObject))
JS_MarkValue(rt, m_location->jsObject, mark_func);
if (JS_IsObject(onerror))
JS_MarkValue(rt, onerror, mark_func);
}
Expand Down
5 changes: 3 additions & 2 deletions bridge/bindings/qjs/bom/window.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,13 @@ class WindowInstance : public EventTargetInstance {
public:
WindowInstance() = delete;
explicit WindowInstance(Window* window);
~WindowInstance() { JS_FreeValue(m_ctx, onerror); }
~WindowInstance() {}

private:
void gcMark(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) override;

Location* m_location{new Location(m_context)};
ObjectProperty m_location{m_context, instanceObject, "m_location", (new Location(m_context))->jsObject};
ObjectProperty m_onerror{m_context, instanceObject, "m_onerror", JS_NULL};
JSValue onerror{JS_NULL};
friend Window;
friend JSContext;
Expand Down
65 changes: 26 additions & 39 deletions bridge/bindings/qjs/dom/event_target.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,18 +69,18 @@ JSValue EventTarget::addEventListener(QjsContext* ctx, JSValue this_val, int arg
JSAtom eventTypeAtom = JS_ValueToAtom(ctx, eventTypeValue);

// Init list.
if (!JS_HasProperty(ctx, eventTargetInstance->m_eventHandlers, eventTypeAtom)) {
if (!JS_HasProperty(ctx, eventTargetInstance->m_eventHandlers.value(), eventTypeAtom)) {
JS_DupAtom(ctx, eventTypeAtom);
auto* atomJob = new AtomJob{eventTypeAtom};
list_add_tail(&atomJob->link, &eventTargetInstance->m_context->atom_job_list);
JS_SetProperty(ctx, eventTargetInstance->m_eventHandlers, eventTypeAtom, JS_NewArray(ctx));
JS_SetProperty(ctx, eventTargetInstance->m_eventHandlers.value(), eventTypeAtom, JS_NewArray(ctx));
}

JSValue eventHandlers = JS_GetProperty(ctx, eventTargetInstance->m_eventHandlers, eventTypeAtom);
JSValue eventHandlers = JS_GetProperty(ctx, eventTargetInstance->m_eventHandlers.value(), eventTypeAtom);
int32_t eventHandlerLen = arrayGetLength(ctx, eventHandlers);

// Dart needs to be notified for the first registration event.
if (eventHandlerLen == 0 || JS_HasProperty(ctx, eventTargetInstance->m_propertyEventHandler, eventTypeAtom)) {
if (eventHandlerLen == 0 || JS_HasProperty(ctx, eventTargetInstance->m_propertyEventHandler.value(), eventTypeAtom)) {
int32_t contextId = eventTargetInstance->prototype()->contextId();

NativeString args_01{};
Expand Down Expand Up @@ -114,12 +114,12 @@ JSValue EventTarget::removeEventListener(QjsContext* ctx, JSValue this_val, int

JSAtom eventTypeAtom = JS_ValueToAtom(ctx, eventTypeValue);

if (!JS_HasProperty(ctx, eventTargetInstance->m_eventHandlers, eventTypeAtom)) {
if (!JS_HasProperty(ctx, eventTargetInstance->m_eventHandlers.value(), eventTypeAtom)) {
JS_FreeAtom(ctx, eventTypeAtom);
return JS_UNDEFINED;
}

JSValue eventHandlers = JS_GetProperty(ctx, eventTargetInstance->m_eventHandlers, eventTypeAtom);
JSValue eventHandlers = JS_GetProperty(ctx, eventTargetInstance->m_eventHandlers.value(), eventTypeAtom);
int32_t targetIdx = arrayFindIdx(ctx, eventHandlers, callback);

if (targetIdx != -1) {
Expand All @@ -128,7 +128,7 @@ JSValue EventTarget::removeEventListener(QjsContext* ctx, JSValue this_val, int

int32_t eventHandlersLen = arrayGetLength(ctx, eventHandlers);

if (eventHandlersLen && JS_HasProperty(ctx, eventTargetInstance->m_propertyEventHandler, eventTypeAtom)) {
if (eventHandlersLen && JS_HasProperty(ctx, eventTargetInstance->m_propertyEventHandler.value(), eventTypeAtom)) {
// Dart needs to be notified for handles is empty.
int32_t contextId = eventTargetInstance->prototype()->contextId();

Expand Down Expand Up @@ -207,8 +207,8 @@ bool EventTargetInstance::internalDispatchEvent(EventInstance* eventInstance) {
JS_FreeValue(m_ctx, returnedValue);
};

if (JS_HasProperty(m_ctx, m_eventHandlers, eventTypeAtom)) {
JSValue eventHandlers = JS_GetProperty(m_ctx, m_eventHandlers, eventTypeAtom);
if (JS_HasProperty(m_ctx, m_eventHandlers.value(), eventTypeAtom)) {
JSValue eventHandlers = JS_GetProperty(m_ctx, m_eventHandlers.value(), eventTypeAtom);
int32_t len = arrayGetLength(m_ctx, eventHandlers);

for (int i = 0; i < len; i++) {
Expand All @@ -221,7 +221,7 @@ bool EventTargetInstance::internalDispatchEvent(EventInstance* eventInstance) {
}

// Dispatch event listener white by 'on' prefix property.
if (JS_HasProperty(m_ctx, m_propertyEventHandler, eventTypeAtom)) {
if (JS_HasProperty(m_ctx, m_propertyEventHandler.value(), eventTypeAtom)) {
if (eventType == "error") {
auto _dispatchErrorEvent = [&eventInstance, this, eventType](JSValue& handler) {
JSValue error = JS_GetPropertyStr(m_ctx, eventInstance->instanceObject, "error");
Expand All @@ -240,11 +240,11 @@ bool EventTargetInstance::internalDispatchEvent(EventInstance* eventInstance) {
JS_FreeValue(m_ctx, lineNumberValue);
JS_FreeValue(m_ctx, columnValue);
};
JSValue v = JS_GetProperty(m_ctx, m_propertyEventHandler, eventTypeAtom);
JSValue v = JS_GetProperty(m_ctx, m_propertyEventHandler.value(), eventTypeAtom);
_dispatchErrorEvent(v);
JS_FreeValue(m_ctx, v);
} else {
JSValue v = JS_GetProperty(m_ctx, m_propertyEventHandler, eventTypeAtom);
JSValue v = JS_GetProperty(m_ctx, m_propertyEventHandler.value(), eventTypeAtom);
_dispatchEvent(v);
JS_FreeValue(m_ctx, v);
}
Expand Down Expand Up @@ -285,9 +285,6 @@ EventTargetInstance::~EventTargetInstance() {
#if FLUTTER_BACKEND
getDartMethod()->flushUICommand();
#endif
JS_FreeValue(m_ctx, m_properties);
JS_FreeValue(m_ctx, m_eventHandlers);
JS_FreeValue(m_ctx, m_propertyEventHandler);
}

int EventTargetInstance::hasProperty(QjsContext* ctx, JSValue obj, JSAtom atom) {
Expand All @@ -306,7 +303,7 @@ int EventTargetInstance::hasProperty(QjsContext* ctx, JSValue obj, JSAtom atom)
return !JS_IsNull(eventTarget->getPropertyHandler(p));
}

return JS_HasProperty(ctx, eventTarget->m_properties, atom);
return JS_HasProperty(ctx, eventTarget->m_properties.value(), atom);
}

JSValue EventTargetInstance::getProperty(QjsContext* ctx, JSValue obj, JSAtom atom, JSValue receiver) {
Expand All @@ -328,8 +325,8 @@ JSValue EventTargetInstance::getProperty(QjsContext* ctx, JSValue obj, JSAtom at
return eventTarget->getPropertyHandler(p);
}

if (JS_HasProperty(ctx, eventTarget->m_properties, atom)) {
return JS_GetProperty(ctx, eventTarget->m_properties, atom);
if (JS_HasProperty(ctx, eventTarget->m_properties.value(), atom)) {
return JS_GetProperty(ctx, eventTarget->m_properties.value(), atom);
}

// For plugin elements, try to auto generate properties and functions from dart response.
Expand Down Expand Up @@ -357,14 +354,14 @@ int EventTargetInstance::setProperty(QjsContext* ctx, JSValue obj, JSAtom atom,
if (!p->is_wide_char && p->u.str8[0] == 'o' && p->u.str8[1] == 'n') {
eventTarget->setPropertyHandler(p, value);
} else {
if (!JS_HasProperty(ctx, eventTarget->m_properties, atom)) {
if (!JS_HasProperty(ctx, eventTarget->m_properties.value(), atom)) {
auto* atomJob = new AtomJob{atom};
list_add_tail(&atomJob->link, &eventTarget->m_context->atom_job_list);
// Increase one reference count for atom to hold this atom value until eventTarget disposed.
JS_DupAtom(ctx, atom);
}

JS_SetProperty(ctx, eventTarget->m_properties, atom, JS_DupValue(ctx, value));
JS_SetProperty(ctx, eventTarget->m_properties.value(), atom, JS_DupValue(ctx, value));

if (isJavaScriptExtensionElementInstance(eventTarget->context(), eventTarget->instanceObject) && !p->is_wide_char && p->u.str8[0] != '_') {
NativeString* args_01 = atomToNativeString(ctx, atom);
Expand Down Expand Up @@ -409,7 +406,7 @@ void EventTargetInstance::setPropertyHandler(JSString* p, JSValue value) {
if (JS_IsNull(value)) {
JS_FreeAtom(m_ctx, atom);
list_del(&atomJob->link);
JS_DeleteProperty(m_ctx, m_propertyEventHandler, atom, 0);
JS_DeleteProperty(m_ctx, m_propertyEventHandler.value(), atom, 0);
return;
}

Expand All @@ -420,9 +417,8 @@ void EventTargetInstance::setPropertyHandler(JSString* p, JSValue value) {
}

JSValue newCallback = JS_DupValue(m_ctx, value);
JS_SetProperty(m_ctx, m_propertyEventHandler, atom, newCallback);

int32_t eventHandlerLen = arrayGetLength(m_ctx, m_eventHandlers);
JS_SetProperty(m_ctx, m_propertyEventHandler.value(), atom, newCallback);
int32_t eventHandlerLen = arrayGetLength(m_ctx, m_eventHandlers.value());
if (eventHandlerLen == 0) {
int32_t contextId = m_context->getContextId();
NativeString* args_01 = atomToNativeString(m_ctx, atom);
Expand All @@ -435,10 +431,10 @@ JSValue EventTargetInstance::getPropertyHandler(JSString* p) {
char eventType[p->len + 1 - 2];
memcpy(eventType, &p->u.str8[2], p->len + 1 - 2);
JSAtom atom = JS_NewAtom(m_ctx, eventType);
if (!JS_HasProperty(m_ctx, m_propertyEventHandler, atom)) {
if (!JS_HasProperty(m_ctx, m_propertyEventHandler.value(), atom)) {
return JS_NULL;
}
return JS_GetProperty(m_ctx, m_propertyEventHandler, atom);
return JS_GetProperty(m_ctx, m_propertyEventHandler.value(), atom);
}

void EventTargetInstance::finalize(JSRuntime* rt, JSValue val) {
Expand All @@ -453,27 +449,18 @@ JSValue EventTargetInstance::getNativeProperty(const char* prop) {
return result;
}

void EventTargetInstance::gcMark(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) {
// Tell gc eventTargetInstance have these properties.
// Should check object is already inited before gc mark.
if (JS_IsObject(m_eventHandlers))
JS_MarkValue(rt, m_eventHandlers, mark_func);
if (JS_IsObject(m_propertyEventHandler))
JS_MarkValue(rt, m_propertyEventHandler, mark_func);
if (JS_IsObject(m_properties))
JS_MarkValue(rt, m_properties, mark_func);
}
void EventTargetInstance::gcMark(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) {}
Copy link
Contributor

@yuanyan yuanyan Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个函数是空的，需要注释说明下，目前c++代码注释是比较少的


void EventTargetInstance::copyNodeProperties(EventTargetInstance* newNode, EventTargetInstance* referenceNode) {
QjsContext* ctx = referenceNode->m_ctx;
JSValue propKeys = objectGetKeys(ctx, referenceNode->m_properties);
JSValue propKeys = objectGetKeys(ctx, referenceNode->m_properties.value());
uint32_t propKeyLen = arrayGetLength(ctx, propKeys);

for (int i = 0; i < propKeyLen; i++) {
JSValue k = JS_GetPropertyUint32(ctx, propKeys, i);
JSAtom kt = JS_ValueToAtom(ctx, k);
JSValue v = JS_GetProperty(ctx, referenceNode->m_properties, kt);
JS_SetProperty(ctx, newNode->m_properties, kt, JS_DupValue(ctx, v));
JSValue v = JS_GetProperty(ctx, referenceNode->m_properties.value(), kt);
JS_SetProperty(ctx, newNode->m_properties.value(), kt, JS_DupValue(ctx, v));

JS_FreeAtom(ctx, kt);
JS_FreeValue(ctx, k);
Expand Down
6 changes: 3 additions & 3 deletions bridge/bindings/qjs/dom/event_target.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ class EventTargetInstance : public Instance {

protected:
int32_t m_eventTargetId;
JSValue m_eventHandlers{JS_NewObject(m_ctx)};
JSValue m_propertyEventHandler{JS_NewObject(m_ctx)};
JSValue m_properties{JS_NewObject(m_ctx)};
ObjectProperty m_eventHandlers{m_context, instanceObject, "__eventHandlers", JS_NewObject(m_ctx)};
ObjectProperty m_propertyEventHandler{m_context, instanceObject, "__propertyEventHandler", JS_NewObject(m_ctx)};
ObjectProperty m_properties{m_context, instanceObject, "__properties", JS_NewObject(m_ctx)};

void gcMark(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) override;
static void copyNodeProperties(EventTargetInstance* newNode, EventTargetInstance* referenceNode);
Expand Down
7 changes: 6 additions & 1 deletion bridge/bindings/qjs/js_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,14 @@ class ObjectProperty {
JS_DefineProperty(context->ctx(), thisObject, key, JS_UNDEFINED, get, JS_UNDEFINED, JS_PROP_HAS_CONFIGURABLE | JS_PROP_ENUMERABLE | JS_PROP_HAS_GET);
JS_FreeAtom(context->ctx(), key);
}
explicit ObjectProperty(JSContext* context, JSValueConst thisObject, const char* property, JSValue value) {
explicit ObjectProperty(JSContext* context, JSValueConst thisObject, const char* property, JSValue value) : m_value(value) {
JS_DefinePropertyValueStr(context->ctx(), thisObject, property, value, JS_PROP_ENUMERABLE);
}

JSValue value() { return m_value; }

private:
JSValue m_value{JS_NULL};
};

class ObjectFunction {
Expand Down