Skip to content

Commit

Permalink
WebIDL: Implement Owner Extended Attribute, fix libass#77
Browse files Browse the repository at this point in the history 
Currently the ensureCache create a temporary pointer that will
transfer the value to the WASM/C part and recycle it with to use in the
next value, but since the libass struct pointers are owned by the
library, the pointers can't be recycled or freed or can lead into
an undefined behavior.

This configure the binder to tranfer the pointer ownership instead of
recycle it.

To avoid creating complex code, I decided to fix it in the webidl
binder.
  • Loading branch information
@TFSThiagoBR98
TFSThiagoBR98 committed Mar 13, 2022
1 parent d1072f3 commit af1dc9a
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 36 deletions.
4 changes: 3 additions & 1 deletion build/WebIDL.py
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## JavascriptSubtitlesOctopus
## Patched to:
## - add integer pointers (IntPtr)
## - add [Owner] Extended attribute
## From https://github.com/emscripten-core/emscripten/blob/f36f9fcaf83db93e6a6d0f0cdc47ab6379ade139/third_party/WebIDL.py

# from https://hg.mozilla.org/mozilla-central/file/tip/dom/bindings/parser/WebIDL.py
Expand Down Expand Up @@ -2863,6 +2864,7 @@ def handleExtendedAttribute(self, attr):
identifier == "AvailableIn" or
identifier == "Const" or
identifier == "Value" or
identifier == "Owner" or
identifier == "BoundsChecked" or
identifier == "NewObject"):
# Known attributes that we don't need to do anything with here
Expand Down Expand Up @@ -2938,7 +2940,7 @@ def addExtendedAttributes(self, attrs):
self.enforceRange = True
elif identifier == "TreatNonCallableAsNull":
self._allowTreatNonCallableAsNull = True
elif identifier in ['Ref', 'Const']:
elif identifier in ['Ref', 'Const', 'Owner']:
# ok in emscripten
self._extraAttributes[identifier] = True
else:
Expand Down
78 changes: 52 additions & 26 deletions build/webidl_binder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
## Patched to:
## - add integer pointers (IntPtr)
## - implement ByteString type
## - add [Owner] Extended attribute for Pointer Ownership transfer
## From https://github.com/emscripten-core/emscripten/blob/f36f9fcaf83db93e6a6d0f0cdc47ab6379ade139/tools/webidl_binder.py

# Copyright 2014 The Emscripten Authors. All rights reserved.
Expand Down Expand Up @@ -174,6 +175,7 @@ def build_constructor(name):
size: 0, // the size of buffer
pos: 0, // the next free offset in buffer
temps: [], // extra allocations
owned: [], // Owned allocations
needed: 0, // the total size we need next time
prepare: function() {
Expand All @@ -197,22 +199,29 @@ def build_constructor(name):
}
ensureCache.pos = 0;
},
alloc: function(array, view) {
alloc: function(array, view, owner) {
assert(ensureCache.buffer);
var bytes = view.BYTES_PER_ELEMENT;
var len = array.length * bytes;
len = (len + 7) & -8; // keep things aligned to 8 byte boundaries
var ret;
if (ensureCache.pos + len >= ensureCache.size) {
// we failed to allocate in the buffer, ensureCache time around :(
if (owner) {
assert(len > 0); // null terminator, at least
ensureCache.needed += len;
ret = Module['_malloc'](len);
ensureCache.temps.push(ret);
ensureCache.owned.push(ret);
} else {
// we can allocate in the buffer
ret = ensureCache.buffer + ensureCache.pos;
ensureCache.pos += len;
if (ensureCache.pos + len >= ensureCache.size) {
// we failed to allocate in the buffer, ensureCache time around :(
assert(len > 0); // null terminator, at least
ensureCache.needed += len;
ret = Module['_malloc'](len);
ensureCache.temps.push(ret);
} else {
// we can allocate in the buffer
ret = ensureCache.buffer + ensureCache.pos;
ensureCache.pos += len;
}
}
return ret;
},
Expand All @@ -228,58 +237,73 @@ def build_constructor(name):
view[offset + i] = array[i];
}
},
clear: function(clearOwned) {
for (var i = 0; i < ensureCache.temps.length; i++) {
Module['_free'](ensureCache.temps[i]);
}
if (clearOwned) {
for (var i = 0; i < ensureCache.owned.length; i++) {
Module['_free'](ensureCache.owned[i]);
}
}
ensureCache.temps.length = 0;
Module['_free'](ensureCache.buffer);
ensureCache.buffer = 0;
ensureCache.size = 0;
ensureCache.needed = 0;
}
};
/** @suppress {duplicate} (TODO: avoid emitting this multiple times, it is redundant) */
function ensureString(value) {
function ensureString(value, owner) {
if (typeof value === 'string') {
var intArray = intArrayFromString(value);
var offset = ensureCache.alloc(intArray, HEAP8);
var offset = ensureCache.alloc(intArray, HEAP8, owner);
ensureCache.copy(intArray, HEAP8, offset);
return offset;
}
return value;
}
/** @suppress {duplicate} (TODO: avoid emitting this multiple times, it is redundant) */
function ensureInt8(value) {
function ensureInt8(value, owner) {
if (typeof value === 'object') {
var offset = ensureCache.alloc(value, HEAP8);
var offset = ensureCache.alloc(value, HEAP8, owner);
ensureCache.copy(value, HEAP8, offset);
return offset;
}
return value;
}
/** @suppress {duplicate} (TODO: avoid emitting this multiple times, it is redundant) */
function ensureInt16(value) {
function ensureInt16(value, owner) {
if (typeof value === 'object') {
var offset = ensureCache.alloc(value, HEAP16);
var offset = ensureCache.alloc(value, HEAP16, owner);
ensureCache.copy(value, HEAP16, offset);
return offset;
}
return value;
}
/** @suppress {duplicate} (TODO: avoid emitting this multiple times, it is redundant) */
function ensureInt32(value) {
function ensureInt32(value, owner) {
if (typeof value === 'object') {
var offset = ensureCache.alloc(value, HEAP32);
var offset = ensureCache.alloc(value, HEAP32, owner);
ensureCache.copy(value, HEAP32, offset);
return offset;
}
return value;
}
/** @suppress {duplicate} (TODO: avoid emitting this multiple times, it is redundant) */
function ensureFloat32(value) {
function ensureFloat32(value, owner) {
if (typeof value === 'object') {
var offset = ensureCache.alloc(value, HEAPF32);
var offset = ensureCache.alloc(value, HEAPF32, owner);
ensureCache.copy(value, HEAPF32, offset);
return offset;
}
return value;
}
/** @suppress {duplicate} (TODO: avoid emitting this multiple times, it is redundant) */
function ensureFloat64(value) {
function ensureFloat64(value, owner) {
if (typeof value === 'object') {
var offset = ensureCache.alloc(value, HEAPF64);
var offset = ensureCache.alloc(value, HEAPF64, owner);
ensureCache.copy(value, HEAPF64, offset);
return offset;
}
Expand Down Expand Up @@ -390,7 +414,7 @@ def type_to_cdec(raw):

def render_function(class_name, func_name, sigs, return_type, non_pointer,
copy, operator, constructor, func_scope,
call_content=None, const=False, array_attribute=False):
call_content=None, const=False, owned=False, array_attribute=False):
legacy_mode = CHECKS not in ['ALL', 'FAST']
all_checks = CHECKS == 'ALL'

Expand Down Expand Up @@ -505,20 +529,20 @@ def render_function(class_name, func_name, sigs, return_type, non_pointer,
if not (arg.type.isArray() and not array_attribute):
body += " if ({0} && typeof {0} === 'object') {0} = {0}.ptr;\n".format(js_arg)
if arg.type.isString():
body += " else {0} = ensureString({0});\n".format(js_arg)
body += " else {0} = ensureString({0}, {1});\n".format(js_arg, "true" if (owned) else "false")
else:
# an array can be received here
arg_type = arg.type.name
if arg_type in ['Byte', 'Octet']:
body += " if (typeof {0} == 'object') {{ {0} = ensureInt8({0}); }}\n".format(js_arg)
body += " if (typeof {0} == 'object') {{ {0} = ensureInt8({0}, {1}); }}\n".format(js_arg, "true" if (owned) else "false")
elif arg_type in ['Short', 'UnsignedShort']:
body += " if (typeof {0} == 'object') {{ {0} = ensureInt16({0}); }}\n".format(js_arg)
body += " if (typeof {0} == 'object') {{ {0} = ensureInt16({0}, {1}); }}\n".format(js_arg, "true" if (owned) else "false")
elif arg_type in ['Long', 'UnsignedLong']:
body += " if (typeof {0} == 'object') {{ {0} = ensureInt32({0}); }}\n".format(js_arg)
body += " if (typeof {0} == 'object') {{ {0} = ensureInt32({0}, {1}); }}\n".format(js_arg, "true" if (owned) else "false")
elif arg_type == 'Float':
body += " if (typeof {0} == 'object') {{ {0} = ensureFloat32({0}); }}\n".format(js_arg)
body += " if (typeof {0} == 'object') {{ {0} = ensureFloat32({0}, {1}); }}\n".format(js_arg, "true" if (owned) else "false")
elif arg_type == 'Double':
body += " if (typeof {0} == 'object') {{ {0} = ensureFloat64({0}); }}\n".format(js_arg)
body += " if (typeof {0} == 'object') {{ {0} = ensureFloat64({0}, {1}); }}\n".format(js_arg, "true" if (owned) else "false")

c_names = {}
for i in range(min_args, max_args):
Expand Down Expand Up @@ -737,6 +761,7 @@ def render_function(class_name, func_name, sigs, return_type, non_pointer,
func_scope=interface,
call_content=get_call_content,
const=m.getExtendedAttribute('Const'),
owned=m.getExtendedAttribute('Owner'),
array_attribute=m.type.isArray())

if m.readonly:
Expand All @@ -755,6 +780,7 @@ def render_function(class_name, func_name, sigs, return_type, non_pointer,
func_scope=interface,
call_content=set_call_content,
const=m.getExtendedAttribute('Const'),
owned=m.getExtendedAttribute('Owner'),
array_attribute=m.type.isArray())
mid_js += [r'''
Object.defineProperty(%s.prototype, '%s', { get: %s.prototype.%s, set: %s.prototype.%s });''' % (name, attr, name, get_name, name, set_name)]
Expand Down
18 changes: 9 additions & 9 deletions src/SubtitleOctopus.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ interface ASS_Image {

[NoDelete]
interface ASS_Style {
attribute DOMString Name;
attribute DOMString FontName;
[Owner] attribute DOMString Name;
[Owner] attribute DOMString FontName;
attribute double FontSize;
attribute unsigned long PrimaryColour;
attribute unsigned long SecondaryColour;
Expand Down Expand Up @@ -47,12 +47,12 @@ interface ASS_Event {
attribute long ReadOrder;
attribute long Layer;
attribute long Style;
attribute DOMString Name;
[Owner] attribute DOMString Name;
attribute long MarginL;
attribute long MarginR;
attribute long MarginV;
attribute DOMString Effect;
attribute DOMString Text;
[Owner] attribute DOMString Effect;
[Owner] attribute DOMString Text;
};

[NoDelete]
Expand All @@ -63,17 +63,17 @@ interface ASS_Track {
attribute long max_events;
[Value] attribute ASS_Style[] styles;
[Value] attribute ASS_Event[] events;
attribute DOMString style_format;
attribute DOMString event_format;
[Owner] attribute DOMString style_format;
[Owner] attribute DOMString event_format;
attribute long PlayResX;
attribute long PlayResY;
attribute double Timer;
attribute long WrapStyle;
attribute long ScaledBorderAndShadow;
attribute long Kerning;
attribute DOMString Language;
[Owner] attribute DOMString Language;
attribute long default_style;
attribute DOMString name;
[Owner] attribute DOMString name;
};

enum ASS_Hinting {
Expand Down

0 comments on commit af1dc9a

Please sign in to comment.