Don't use expando properties for storing data on places nodes

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: mano)

Tracking

(Blocks: 1 bug, {addon-compat, perf})

Trunk
Firefox 15
addon-compat, perf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15+ fixed)

Details

Attachments

(7 attachments, 9 obsolete attachments)

4.80 KB, patch
mak
: review+
mano
: checkin+
Details | Diff | Splinter Review
829 bytes, patch
mak
: review+
mano
: checkin+
Details | Diff | Splinter Review
25.15 KB, patch
mano
: checkin+
Details | Diff | Splinter Review
29.53 KB, patch
mano
: checkin+
Details | Diff | Splinter Review
2.99 KB, patch
mano
: checkin+
Details | Diff | Splinter Review
4.24 KB, patch
mak
: review+
mano
: checkin+
Details | Diff | Splinter Review
47.92 KB, patch
mano
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Mano suggested this to me yesterday, and sounds like an awesome idea.

Currently we attach data to nodes through expando properties, and this adds some complexity to our code.
Moreover it may increase the work needed for GC+CC, since we have cycles like placesNode._DOMElement._placesNode.
I think this idea is very good.

The WeakMap references in MDN describes that current WeakMap implementation is not stable and we do not rely on it for production code.
(https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/WeakMap)

We have to wait that WeakMap will be implemented as stable?
As far as I understand, the implementation is stable, but the API isn't, but that's not such a concern for in-tree code.

Taking.
Assignee: nobody → mano
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
Blocks: 569127
(Reporter)

Updated

5 years ago
Blocks: 714689
Nominating for tracking per Mano's suggestion, since compartment-per-global makes this more of a problem.
tracking-firefox15: --- → ?
Tracking flags aren't a way to ask for bugs to be prioritized, they're just a way to indicate that release-management folks need to monitor the bug before that train ships, either because they're new features that may need disabling, regressions, critical security issues, etc. None of that seems to apply here.
tracking-firefox15: ? → ---
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #4)
> Tracking flags aren't a way to ask for bugs to be prioritized, they're just
> a way to indicate that release-management folks need to monitor the bug
> before that train ships, either because they're new features that may need
> disabling, regressions, critical security issues, etc. None of that seems to
> apply here.

The issue is that places is doing things that are not compartment-per-global safe (putting expandos on per-compartment XPCWNs), and we're landing CPG. The tests are green, but we could be turning on a whole class of hidden bugs (and therefore regressions) here. So I thought we should make sure that we actually followed through with this before the next release.

It's your call though. Re-nominating for tracking, not because I necessarily disagree with tracking-, but just because I want to make sure we're on the same page (and because the bug previously lacked context).
tracking-firefox15: --- → ?
(Reporter)

Comment 6

5 years ago
We need to better track prioritization of bugs, instead tracking should be set on the CPG bug, and the idea is that the new feature is disabled/backed-out, rather than "the bug X caused by new feature Y should be fixed before we ship Y", especially in the rapid release process.  I can see how backing out CPG may be problematic and block a bunch of awesome work, so may not be the perfect choice here.

Regarding this bug, fixing it involves rewriting a bunch of code in the views, that may as well introduce a bunch of hidden and new bugs, so no doubt both situations involve regression risks.  I don't think should be a blocker to CPG, though I think its importance grows quite a bit and should be considered a P1 in Places (unfortunately we don't have a good history of using Importance fields).  We may need some more human resources, btw will check availability with Mano and figure out something.
> The tests are green, but we could be turning on a whole class of hidden bugs
> (and therefore regressions) here. So I thought we should make sure that we
> actually followed through with this before the next release.
> 

Tracking for 15 so we can check in again later on bholley's statements here and ensure that they are addressed prior to the release of 15.
tracking-firefox15: ? → +
Created attachment 627791 [details] [diff] [review]
part 1 - _DOMElement

I think we should land this in pieces. 

This is the easiest, though largest, part of this change.
Attachment #627791 - Flags: review?(mak77)
Created attachment 627884 [details] [diff] [review]
part 1 - _DOMElement
Attachment #627791 - Attachment is obsolete: true
Attachment #627791 - Flags: review?(mak77)
Summary: Use weakmaps to track nodes data in Places views, instead of expando properties → Don't use expando properties for storing data on places nodes
Created attachment 627911 [details] [diff] [review]
part 2 - "transparent" property bag for when it's really necessary (test included)

Expose has/get/set/delete (similar to the Map api). I deliberately chose to hide the semantics of nsIPropertyBag, as they don't work so well for js.

This is one option. Another options is to expose a "nodesProperties" *js Map* getter, which would just return a js Map. I'm not sure which one I dislike more...
Attachment #627911 - Flags: review?(mak77)
Attachment #627884 - Flags: review?(mak77)
Created attachment 627912 [details] [diff] [review]
part 3 - feedURI using this new api (wip, toolbar and menu views not fixed)
Attachment #627912 - Flags: feedback?(mak77)
Created attachment 627917 [details] [diff] [review]
Remove expandos support

This will be the last part to land (to be clear, few pieces are still missing).
Attachment #627917 - Flags: review?(mak77)
Created attachment 627918 [details] [diff] [review]
Fix test_sort-date-site-grouping test

Expandos-removeal revealed a bug in this test.
Attachment #627918 - Flags: review?(mak77)
(Reporter)

Comment 14

5 years ago
Comment on attachment 627884 [details] [diff] [review]
part 1 - _DOMElement

Review of attachment 627884 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +100,5 @@
> +    let node = this._domNodes.get(aPlacesNode, null);
> +    if (!node && aThrowIfNotSet !== false)
> +      throw new Error("No DOM node set for aPlacesNode");
> +    return node;
> +  },

Wouldn't be better to have 2 separate helpers instead of a blind boolean argument that everytime I will have to check to know if it means to throw or not and what's the default? this kind of boolean arguments are error-prone imo.

Looks like the "don't throw" use-case is so rare that adding an _hasDOMNodeForPlacesNode shouldn't have bad consequences.
Probably in the end I'm not sure I like hiding the code flow into a getter, the previous code was repetitive, but clear (just think of NS_ENSURE_SUCCESS that hides the fact it warns and returns).
So imo, either we go back to the old explicit flow, or we just implement a throwing get and use .has() below where we should not throw.

@@ +1032,5 @@
>        }
>      }
>  
>      button._placesNode = aChild;
> +    if (!this._getDOMNodeForPlacesNode(aChild, false))

may use .has
(Reporter)

Comment 15

5 years ago
Comment on attachment 627911 [details] [diff] [review]
part 2 - "transparent" property bag for when it's really necessary (test included)

Review of attachment 627911 [details] [diff] [review]:
-----------------------------------------------------------------

maybe a dumb question, but I don't remember. If we expose this, may someone pass an xpcom object as a value? cause the propertybag has a setAsInterface, but I think for our needs we should only allow natives, to avoid possible leaks. Basically I think we should disallow passing an nsISupports.

::: toolkit/components/places/nsINavHistoryService.idl
@@ +158,5 @@
> +   *        the property name
> +   * @return the value for the property aName, if set, "undefined" otherwise
> +   * (void-type nsIVariant).
> +   */
> +  nsIVariant get(in AString aName);

I wonder if we should make these methods clearer, while on a map get,set,has,delete is clear, on a places node their effect is completely unclear.
My guess is that we should at least make these setProperty, hasProperty and so on... property is not exactly correct but can't find a better naming atm.
Or as discussed on irc having a .properties.set/has/get/delete

::: toolkit/components/places/nsNavHistoryResult.cpp
@@ +358,5 @@
> +nsNavHistoryResultNode::Set(const nsAString& aName, nsIVariant* aValue) {
> +  if (!mPropertyBag) {
> +    nsresult rv;
> +    mPropertyBag = do_CreateInstance("@mozilla.org/hash-property-bag;1", &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);

nit: may use NS_ENSURE_TRUE(mPropertyBag and avoid rv

@@ +375,5 @@
> +
> +NS_IMETHODIMP
> +nsNavHistoryResultNode::Delete(const nsAString& aName) {
> +  if (mPropertyBag)
> +    mPropertyBag->DeleteProperty(aName);

please brace. Ideally we should always brace both in cpp and js, but at least we should in cpp.

::: toolkit/components/places/tests/unit/test_node_properties.js
@@ +16,5 @@
> +  do_check_false(node.has("prop"))
> +  do_check_true(node.get("prop") === undefined);
> +
> +  // Make sure this doesn't throw.
> +  node.delete("prop");

please close the node, for the usual fact add-ons copy our tests code :)

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +121,5 @@
>  [test_PlacesUtils_asyncGetBookmarkIds.js]
>  [test_PlacesUtils_lazyobservers.js]
>  [test_placesTxn.js]
>  [test_telemetry.js]
> +[test_node_properties.js]

please keep tests alphabetical ordered
Attachment #627911 - Flags: review?(mak77)
(Reporter)

Updated

5 years ago
Attachment #627884 - Flags: review?(mak77)
(Reporter)

Updated

5 years ago
Attachment #627912 - Attachment is patch: true
Attachment #627912 - Attachment mime type: application/x-javascript → text/plain
(Reporter)

Comment 16

5 years ago
Comment on attachment 627912 [details] [diff] [review]
part 3 - feedURI using this new api (wip, toolbar and menu views not fixed)

Review of attachment 627912 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/controller.js
@@ +1038,5 @@
>          addData(PlacesUtils.TYPE_X_MOZ_PLACE, i);
>  
>          // Drop the feed uri for livemark containers
> +        if (node.has("feedURI")) {
> +          addURIData(i, node.get("feedURI").QueryInterface(Ci.nsIURI).spec);

store the spec so we avoid the QI

@@ +1112,5 @@
>          return;
>        if (PlacesUtils.nodeIsFolder(node))
>          copiedFolders.push(node);
>  
> +      let overrideURI = node.has("feedURI") ? node.get("feedURI").QueryInterface(Ci.nsIURI).spec : null;

ditto

::: browser/components/places/content/treeView.js
@@ +856,5 @@
>        PlacesUtils.livemarks.getLivemark(
>          { id: aNode.itemId },
>          (function (aStatus, aLivemark) {
>            if (Components.isSuccessCode(aStatus)) {
> +            aNode.set("feedURI", aLivemark.feedURI)

let's rather store the .spec and call it feedURL
Attachment #627912 - Flags: feedback?(mak77)
(Reporter)

Comment 17

5 years ago
Comment on attachment 627917 [details] [diff] [review]
Remove expandos support

Review of attachment 627917 [details] [diff] [review]:
-----------------------------------------------------------------

please, do a tryrun once you have all the updated patches. Off-hand I don't remember other things supported by having the result implement classInfo, but with time we added so many things...
Attachment #627917 - Flags: review?(mak77) → review+
(Reporter)

Updated

5 years ago
Keywords: addon-compat
(Reporter)

Updated

5 years ago
Attachment #627918 - Flags: review?(mak77) → review+
Created attachment 629170 [details] [diff] [review]
part 1 - _DOMElement
Attachment #627884 - Attachment is obsolete: true
Attachment #629170 - Flags: review?(mak77)
(Reporter)

Comment 19

5 years ago
Comment on attachment 629170 [details] [diff] [review]
part 1 - _DOMElement

Review of attachment 629170 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +92,5 @@
> +  _getDOMNodeForPlacesNode:
> +  function PVB__getDOMNodeForPlacesNode(aPlacesNode) {
> +    let node = this._domNodes.get(aPlacesNode, null);
> +    if (!node)
> +      throw new Error("No DOM node set for aPlacesNode");

wonder if would be nice to print out something about the node, like its type and parent (to check it's alive)... something useful for debugging
Attachment #629170 - Flags: review?(mak77) → review+
Comment on attachment 627911 [details] [diff] [review]
part 2 - "transparent" property bag for when it's really necessary (test included)

Not going with this approach after all.
Attachment #627911 - Attachment is obsolete: true
Attachment #627912 - Attachment is obsolete: true
Created attachment 629229 [details] [diff] [review]
part 2 - cellProperties, plainContainer
Attachment #629229 - Flags: review?(mak77)
Whiteboard: [Leave open after merge]
(Reporter)

Comment 22

5 years ago
Comment on attachment 629229 [details] [diff] [review]
part 2 - cellProperties, plainContainer

Review of attachment 629229 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/treeView.js
@@ +113,1 @@
>              nodeType != Ci.nsINavHistoryResultNode.RESULT_TYPE_FOLDER_SHORTCUT);

the parenthesis are useless now

@@ +853,5 @@
>          { id: aNode.itemId },
>          (function (aStatus, aLivemark) {
>            if (Components.isSuccessCode(aStatus)) {
>              aNode._feedURI = aLivemark.feedURI;
> +            let properties = this._cellProperties.get(aNode, null);

fwiw, weakmap returns undefined if a defaultValue is not suggested (the second arg to get is optional indeed), did you always pass null for consistency?

@@ +1210,3 @@
>      }
> +    for (let property of properties)
> +      aProperties.AppendElement(property);

please always brace loops, at least :)
Attachment #629229 - Flags: review?(mak77) → review+
(Reporter)

Comment 23

5 years ago
Comment on attachment 629170 [details] [diff] [review]
part 1 - _DOMElement

Review of attachment 629170 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +1025,5 @@
>        }
>      }
>  
>      button._placesNode = aChild;
> +    if (!this._domNodes.has(aChild, button))

just as a reminder, NeilAway found this typo, to be fixed.
Created attachment 629307 [details] [diff] [review]
part 1 + 2 as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/c4bf270b5b0a
Attachment #629170 - Attachment is obsolete: true
Attachment #629229 - Attachment is obsolete: true
Attachment #629307 - Flags: checkin+
Created attachment 629348 [details] [diff] [review]
part 3, livemarks

What a nightmare.
Attachment #629348 - Flags: review?(mak77)
Created attachment 629378 [details] [diff] [review]
part 3 - livemarks, addressing neil's comments
Attachment #629348 - Attachment is obsolete: true
Attachment #629348 - Flags: review?(mak77)
Attachment #629378 - Flags: review?(mak77)
(Reporter)

Comment 27

5 years ago
Comment on attachment 629378 [details] [diff] [review]
part 3 - livemarks, addressing neil's comments

Review of attachment 629378 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following fixes (especially the wrong if conditions!)

::: browser/components/places/content/browserPlacesViews.js
@@ +655,3 @@
>              if (Components.isSuccessCode(aStatus)) {
> +              let shouldInvalidate =
> +                this.controller.hasCachedLivemarkInfo(aPlacesNode);

you have lost a ! here... it was "let shouldInvalidate = !aPlacesNode._feedURI;", so should !hasCached

::: browser/components/places/content/controller.js
@@ +1287,5 @@
>        this._view.selectItems(insertedNodeIds, false);
> +  },
> +
> +  /**
> +   * Cache the livemark info for a node.  This allows the controller

allows the controller and the views

@@ +1292,5 @@
> +   * to treat the given node as a livemark.
> +   * @param aNode
> +   *        a places result node.
> +   * @param aLivemarkInfo
> +   *        a livemark-info object.

mozILivemarkInfo

@@ +1299,5 @@
> +    this._cachedLivemarkInfoObjects.set(aNode, aLivemarkInfo);
> +  },
> +
> +  hasCachedLivemarkInfo: function PC_hasCachedLivemarkInfo(aNode)
> +    this._cachedLivemarkInfoObjects.has(aNode),

You added documentation to the other ones, not this one... not that I care that much, it's self documenting, but sounds a bit strange :)

@@ +1302,5 @@
> +  hasCachedLivemarkInfo: function PC_hasCachedLivemarkInfo(aNode)
> +    this._cachedLivemarkInfoObjects.has(aNode),
> +
> +  /**
> +   * Returns the cached livemark info for a node, if set (by cacheLivemarkInfo),

remove parenthesis

@@ +1306,5 @@
> +   * Returns the cached livemark info for a node, if set (by cacheLivemarkInfo),
> +   * null otherwise.
> +   * @param aNode
> +   *        a places result node.
> +   * @return the livemark-info object for aNode, if set, null otherwise.

mozILivemarkInfo

::: browser/components/places/content/treeView.js
@@ +892,3 @@
>            if (Components.isSuccessCode(aStatus)) {
> +            let shouldInvalidate = 
> +              this._controller.hasCachedLivemarkInfo(aNode);

you lost the ! here as well, should be !hasCached
Attachment #629378 - Flags: review?(mak77) → review+
Created attachment 629469 [details] [diff] [review]
part 3 - livemarks, as checked in

I've also fixed a bug I introduced in PVB_nodeHistroyDetailsChanged in the previous check-in.
Attachment #629378 - Attachment is obsolete: true
Attachment #629469 - Flags: checkin+
Comment on attachment 629469 [details] [diff] [review]
part 3 - livemarks, as checked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/efd698c68283
Comment on attachment 627918 [details] [diff] [review]
Fix test_sort-date-site-grouping test

http://hg.mozilla.org/integration/mozilla-inbound/rev/1f359ffe1cdd
Attachment #627918 - Flags: checkin+
Created attachment 629472 [details] [diff] [review]
Combined front-end changes for SeaMonkey
https://hg.mozilla.org/mozilla-central/rev/c4bf270b5b0a
https://hg.mozilla.org/mozilla-central/rev/efd698c68283
https://hg.mozilla.org/mozilla-central/rev/1f359ffe1cdd
Created attachment 629516 [details] [diff] [review]
Fix typo found by Neil, remove some useless comments

http://hg.mozilla.org/integration/mozilla-inbound/rev/7ee3f3fe90fb
Attachment #629516 - Flags: checkin+
Created attachment 629521 [details] [diff] [review]
_cutting

After removing expandos support, this is last change required for tests to pass.

Marco: Fx13 is about to ship with Set support included, thus I think it's safe to use it, finally.

Note: Sometime soon I'm going to make treeView.js the actual places view for tress, and then this kind of forwarding won't be needed.
Attachment #629521 - Flags: review?(mak77)
Created attachment 629524 [details] [diff] [review]
Combined front-end changes for SeaMonkey, as checked in

http://hg.mozilla.org/comm-central/rev/58c04223b055
Attachment #629472 - Attachment is obsolete: true
Attachment #629524 - Flags: checkin+
(Reporter)

Comment 37

5 years ago
Comment on attachment 629521 [details] [diff] [review]
_cutting

Review of attachment 629521 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this wrappedJSObject addition sucks, but I assume you did it cause being treeview-only would have been bad to use the shared controller.
I don't have better ideas atm.
I also assume you tested this manually in the library and sidebar.
Attachment #629521 - Flags: review?(mak77) → review+
I could avoid this wrappedJSObject thing by simply returning the PlacesTreeView instance without going through treeBoxObject. I didn't do so because changing this |view| getter & setter tended to introduce regression in the past, and we're too close to "release". Anyway, the real fix is getting rid of tree.xml, which I'm going to do sooner than you think... :)
Oh, and thanks for reviewing on Sunday :)
Comment on attachment 629521 [details] [diff] [review]
_cutting

http://hg.mozilla.org/integration/mozilla-inbound/rev/04682cf84deb
Attachment #629521 - Flags: checkin+
Comment on attachment 627917 [details] [diff] [review]
Remove expandos support

http://hg.mozilla.org/integration/mozilla-inbound/rev/b1b44a491f6a

Done.
Attachment #627917 - Flags: checkin+
Whiteboard: [Leave open after merge]
https://hg.mozilla.org/mozilla-central/rev/7ee3f3fe90fb
https://hg.mozilla.org/mozilla-central/rev/04682cf84deb
https://hg.mozilla.org/mozilla-central/rev/b1b44a491f6a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15
(Reporter)

Updated

5 years ago
Depends on: 761027

Updated

5 years ago
Depends on: 760940

Updated

5 years ago
Depends on: 761494
Should I be looking for something other than _DOMElement to find add-on compatibility issues?
(Reporter)

Comment 44

5 years ago
(In reply to Jorge Villalobos [:jorgev] from comment #43)
> Should I be looking for something other than _DOMElement to find add-on
> compatibility issues?

that's the major change (also ._feedURI, ._siteURI, ._cellProperties), though most of the changed stuff was considered private (but I'm sure someone copied the views code).
Should we still be considering this as a possible uplift to FF15, given perceived risk vs reward? We originally tracked because of the possibility of hidden regressions. Does that concern still stand?
Alex, this has landed for Fx15 already, afaict.
status-firefox15: --- → fixed
You need to log in before you can comment on or make changes to this bug.