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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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?

Comment 2

[Asaf Romano (gone)]

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.

Comment 3

[Bobby Holley (:bholley)]

Nominating for tracking per Mano's suggestion, since compartment-per-global makes this more of a problem.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 5

[Bobby Holley (:bholley)]

(In reply to :Gavin Sharp (use [e-mail] 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).

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

> 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.

Comment 8

[Asaf Romano (gone)]

Attachment: part 1 - _DOMElement

I think we should land this in pieces. 

This is the easiest, though largest, part of this change.

Comment 9

[Asaf Romano (gone)]

Attachment: part 1 - _DOMElement

Comment 10

[Asaf Romano (gone)]

Attachment: 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...

Comment 11

[Asaf Romano (gone)]

Attachment: part 3 - feedURI using this new api (wip, toolbar and menu views not fixed)

Comment 12

[Asaf Romano (gone)]

Attachment: Remove expandos support

This will be the last part to land (to be clear, few pieces are still missing).

Comment 13

[Asaf Romano (gone)]

Attachment: Fix test_sort-date-site-grouping test

Expandos-removeal revealed a bug in this test.

Comment 14

[Marco Bonardo [:mak]]

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

Comment 15

[Marco Bonardo [:mak]]

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

Comment 16

[Marco Bonardo [:mak]]

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

Comment 17

[Marco Bonardo [:mak]]

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...

Comment 18

[Asaf Romano (gone)]

Attachment: part 1 - _DOMElement

Comment 19

[Marco Bonardo [:mak]]

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

Comment 20

[Asaf Romano (gone)]

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.

Comment 21

[Asaf Romano (gone)]

Attachment: part 2 - cellProperties, plainContainer

Comment 22

[Marco Bonardo [:mak]]

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 :)

Comment 23

[Marco Bonardo [:mak]]

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.

Comment 24

[Asaf Romano (gone)]

Attachment: part 1 + 2 as checked in

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

Comment 25

[Asaf Romano (gone)]

Attachment: part 3, livemarks

What a nightmare.

Comment 26

[Asaf Romano (gone)]

Attachment: part 3 - livemarks, addressing neil's comments

Comment 27

[Marco Bonardo [:mak]]

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

Comment 28

[Asaf Romano (gone)]

Attachment: part 3 - livemarks, as checked in

I've also fixed a bug I introduced in PVB_nodeHistroyDetailsChanged in the previous check-in.

Comment 29

[Asaf Romano (gone)]

Comment on attachment 629469 [details] [diff] [review]
part 3 - livemarks, as checked in

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

Comment 30

[Asaf Romano (gone)]

Comment on attachment 627918 [details] [diff] [review]
Fix test_sort-date-site-grouping test

http://hg.mozilla.org/integration/mozilla-inbound/rev/1f359ffe1cdd

Comment 31

[Asaf Romano (gone)]

Attachment: Combined front-end changes for SeaMonkey

Comment 32

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c4bf270b5b0a

Comment 33

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/efd698c68283
https://hg.mozilla.org/mozilla-central/rev/1f359ffe1cdd

Comment 34

[Asaf Romano (gone)]

Attachment: Fix typo found by Neil, remove some useless comments

http://hg.mozilla.org/integration/mozilla-inbound/rev/7ee3f3fe90fb

Comment 35

[Asaf Romano (gone)]

Attachment: _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.

Comment 36

[Asaf Romano (gone)]

Attachment: Combined front-end changes for SeaMonkey, as checked in

http://hg.mozilla.org/comm-central/rev/58c04223b055

Comment 37

[Marco Bonardo [:mak]]

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.

Comment 38

[Asaf Romano (gone)]

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... :)

Comment 39

[Asaf Romano (gone)]

Oh, and thanks for reviewing on Sunday :)

Comment 40

[Asaf Romano (gone)]

Comment on attachment 629521 [details] [diff] [review]
_cutting

http://hg.mozilla.org/integration/mozilla-inbound/rev/04682cf84deb

Comment 41

[Asaf Romano (gone)]

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

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

Done.

Comment 42

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7ee3f3fe90fb
https://hg.mozilla.org/mozilla-central/rev/04682cf84deb
https://hg.mozilla.org/mozilla-central/rev/b1b44a491f6a

Comment 43

[Jorge Villalobos [:jorgev] (he/him)]

Should I be looking for something other than _DOMElement to find add-on compatibility issues?

Comment 44

[Marco Bonardo [:mak]]

(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).

Comment 45

[Alex Keybl [:akeybl]]

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?

Comment 46

[Asaf Romano (gone)]

Alex, this has landed for Fx15 already, afaict.