Last modified: 2014-05-16 01:35:05 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T50048, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 48048 - Util.clone doesn't deep-clone KV objects
Util.clone doesn't deep-clone KV objects
Status: NEW
Product: Parsoid
Classification: Unclassified
General (Other open bugs)
unspecified
All All
: Low normal
: ---
Assigned To: Gabriel Wicke
:
: 65281 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-03 14:29 UTC by C. Scott Ananian
Modified: 2014-05-16 01:35 UTC (History)
4 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description C. Scott Ananian 2013-05-03 14:29:15 UTC
Digging into why I couldn't get Util.clone([new KV("a","b")]) to actually return a new KV object, I found that the implementation of $.extend explicitly avoids deep copying any object which was built with a constructor (ie, an object o in which o.__proto__ !== Object.prototype).  The quirks of how we invoke $.extend in Util.clone mean that we get a clone of KV if we invoke Util.clone(kv) directly, but not if KV is embedded in another object or array.

Is this problem well-known?  It seems like we should probably just replace Util.clone with a direct implementation which recurses into objects with prototypes.  That's a bit dangerous as well, since we're not actually copying the prototype and its methods.  Perhaps we should have a whitelist of object types we allow in our Util.clone() implementation, and assert() if some other object type is encountered?
Comment 1 Gerrit Notification Bot 2013-06-11 22:10:22 UTC
Related URL: https://gerrit.wikimedia.org/r/68114 (Gerrit Change Iffaf8c6617f8df894ddb45327992a28d59b40fec)
Comment 2 Gabriel Wicke 2013-06-11 22:52:20 UTC
I implemented a version with support for constructor-based objects, but that runs into issues with cycles in our data structures. Cycles are currently mainly built via constructor-based objects, so not cloning those avoids the problem.

A better solution seems to be to use cloneNode and clone member methods for constructor-based objects. This is not a general solution for cycles either, but at least the issue can be worked around manually in the custom clone method.
Comment 3 Mark A. Hershberger 2014-05-16 01:35:05 UTC
*** Bug 65281 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links