diff options
| -rw-r--r-- | jstests/core/bson_compare_bug.js | 47 | ||||
| -rw-r--r-- | src/mongo/scripting/mozjs/db.cpp | 2 | ||||
| -rw-r--r-- | src/mongo/scripting/mozjs/objectwrapper.cpp | 42 | ||||
| -rw-r--r-- | src/mongo/scripting/mozjs/objectwrapper.h | 7 |
4 files changed, 96 insertions, 2 deletions
diff --git a/jstests/core/bson_compare_bug.js b/jstests/core/bson_compare_bug.js new file mode 100644 index 00000000000..2a39efd8db7 --- /dev/null +++ b/jstests/core/bson_compare_bug.js @@ -0,0 +1,47 @@ +(function() { + "use strict"; + + db.bson_compare_bug.drop(); + + // We want some BSON objects for this test. One convenient way to get that is to insert them + // into the database and then get them back through a query. + const coll = db.bson_compare_bug; + assert.commandWorked(coll.insert( + [ + {_id: 1, obj: {val: [], _id: 1}}, + {_id: 2, obj: {val: []}}, + {_id: 3, obj: {_id: 1, val: []}} + ], + {writeConcern: {w: "majority"}})); + + // The $replaceRoot is so we can get back two results that have an "_id" field and one that + // doesn't. The first two results from this query are the same, except for that. + // res[0]: {val: [], _id: 1} + // res[1]: {val: []} + const res = coll.aggregate([{$sort: {_id: 1}}, {$replaceRoot: {newRoot: "$obj"}}]).toArray(); + assert.eq(3, res.length); + + // bsonBinaryEqual() should see that the BSON results from the query are not equal. + assert(!bsonBinaryEqual(res[0], res[1])); + + // A magic trick: the shell represents the objects in res[0] and res[1] as JavaScript objects + // that internally store raw BSON data but also maintain JavaScript properties for each of their + // BSON fields. The BSON and JavaScript properties are kept in sync both ways. Reading the "val" + // property for the first time results in a call to BSONInfo::resolve(), which materializes the + // "val" BSON field as a JavaScript property. In this case, the resolve function also + // conservatively marks the object as "altered," because "val" is an array, and there's no way + // to observe modifications to it. + assert.eq(res[0].val, res[1].val); + + // We repeat the BSON comparison, but this time, the objects are "altered," and bsonBinaryEqual + // needs to sync the JavaScript properties back into BSON. Before SERVER-39521, a bug in the + // conversion would ignore the "_id" field unless it was previously resolved, which would cause + // res[0] and res[1] to appear equal. + assert(!bsonBinaryEqual(res[0], res[1])); + + // The bug that caused the "_id" field to get dropped in conversion involves code that is + // supposed to move the "_id" field to the front when converting a JavaScript object to BSON. + // This check ensures that "_id" is still getting moved to the front. The value of res[0] should + // now have changed so that both it and res[2] have their _id field first. + assert(bsonBinaryEqual(res[0], res[2])); +}()); diff --git a/src/mongo/scripting/mozjs/db.cpp b/src/mongo/scripting/mozjs/db.cpp index 228cd3d4acc..4748e9a308b 100644 --- a/src/mongo/scripting/mozjs/db.cpp +++ b/src/mongo/scripting/mozjs/db.cpp @@ -75,7 +75,7 @@ void DBInfo::resolve(JSContext* cx, JS::HandleObject obj, JS::HandleId id, bool* } // Check if this exists on the parent, ie. DBCollection::resolve case - if (parentWrapper.hasOwnField(id)) { + if (parentWrapper.alreadyHasOwnField(id)) { parentWrapper.getValue(id, &coll); o.defineProperty(id, coll, 0); diff --git a/src/mongo/scripting/mozjs/objectwrapper.cpp b/src/mongo/scripting/mozjs/objectwrapper.cpp index 306c1f4ef64..3c57e262029 100644 --- a/src/mongo/scripting/mozjs/objectwrapper.cpp +++ b/src/mongo/scripting/mozjs/objectwrapper.cpp @@ -203,6 +203,41 @@ bool ObjectWrapper::Key::hasOwn(JSContext* cx, JS::HandleObject o) { switch (_type) { case Type::Field: + if (JS_HasOwnProperty(cx, o, _field, &has)) + return has; + break; + case Type::Index: { + JS::RootedId id(cx); + + // This is a little different because there is no JS_HasOwnElement + if (JS_IndexToId(cx, _idx, &id) && JS_HasOwnPropertyById(cx, o, id, &has)) + return has; + break; + } + case Type::Id: { + JS::RootedId id(cx, _id); + + if (JS_HasOwnPropertyById(cx, o, id, &has)) + return has; + break; + } + case Type::InternedString: { + InternedStringId id(cx, _internedString); + + if (JS_HasOwnPropertyById(cx, o, id, &has)) + return has; + break; + } + } + + throwCurrentJSException(cx, ErrorCodes::InternalError, "Failed to hasOwn value on a JSObject"); +} + +bool ObjectWrapper::Key::alreadyHasOwn(JSContext* cx, JS::HandleObject o) { + bool has; + + switch (_type) { + case Type::Field: if (JS_AlreadyHasOwnProperty(cx, o, _field, &has)) return has; break; @@ -226,7 +261,8 @@ bool ObjectWrapper::Key::hasOwn(JSContext* cx, JS::HandleObject o) { } } - throwCurrentJSException(cx, ErrorCodes::InternalError, "Failed to hasOwn value on a JSObject"); + throwCurrentJSException( + cx, ErrorCodes::InternalError, "Failed to alreadyHasOwn value on a JSObject"); } void ObjectWrapper::Key::del(JSContext* cx, JS::HandleObject o) { @@ -456,6 +492,10 @@ bool ObjectWrapper::hasOwnField(Key key) { return key.hasOwn(_context, _object); } +bool ObjectWrapper::alreadyHasOwnField(Key key) { + return key.alreadyHasOwn(_context, _object); +} + void ObjectWrapper::callMethod(const char* field, const JS::HandleValueArray& args, JS::MutableHandleValue out) { diff --git a/src/mongo/scripting/mozjs/objectwrapper.h b/src/mongo/scripting/mozjs/objectwrapper.h index 7516807b130..ea006f82523 100644 --- a/src/mongo/scripting/mozjs/objectwrapper.h +++ b/src/mongo/scripting/mozjs/objectwrapper.h @@ -85,6 +85,7 @@ public: void set(JSContext* cx, JS::HandleObject o, JS::HandleValue value); bool has(JSContext* cx, JS::HandleObject o); bool hasOwn(JSContext* cx, JS::HandleObject o); + bool alreadyHasOwn(JSContext* cx, JS::HandleObject o); void define( JSContext* cx, JS::HandleObject o, unsigned attrs, JSNative getter, JSNative setter); void define(JSContext* cx, JS::HandleObject o, JS::HandleValue value, unsigned attrs); @@ -138,9 +139,15 @@ public: void rename(Key key, const char* to); + // has field walks the prototype heirarchy bool hasField(Key key); + + // has own field checks for the field directly on the object bool hasOwnField(Key key); + // already how own field checks for the field directly on the object, ignoring C++ hooks + bool alreadyHasOwnField(Key key); + void callMethod(const char* name, const JS::HandleValueArray& args, JS::MutableHandleValue out); void callMethod(const char* name, JS::MutableHandleValue out); void callMethod(JS::HandleValue fun, |