777005 - Fix Bookmarks and history menus behave incorrectly due to non-node weak map keys (Port Bug 760940)

Status: VERIFIED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Comment 1

[Philip Chee]

From Bug 760940 Comment 0:
> The Bookmarks and History menus behave incorrectly in the recent nightly build, 
> e.g. deleting a bookmark via the Bookmarks menu is not reflected in the menu 
> (only in the Library). Tested with a clean profile.
> 
> Opening Bookmarks menu causes these warnings:
> 
> Warning: Error: Failed to preserve wrapper of wrapped native weak map key.
> Source File: chrome://browser/content/places/browserPlacesViews.js
> Line: 72, 330, 344
> 
> Opening History menu causes these warnings:
> 
> Warning: Error: Failed to preserve wrapper of wrapped native weak map key.
> Source File: chrome://browser/content/places/browserPlacesViews.js
> Line: 72, 344
> 
> Opening Bookmarks Manager (alias Library) causes several of these warnings:
> 
> Warning: Error: Failed to preserve wrapper of wrapped native weak map key.
> Source File: chrome://browser/content/places/treeView.js
> Line: 1211
>

Comment 2

[Frank Wein [:mcsmurf]]

Attachment: Patch (not tested yet)

Comment 3

[Ian Neal]

Comment on attachment 645471 [details] [diff] [review]
Patch (not tested yet)

>+++ b/suite/common/places/treeView.js
> 
>     let ancestors = PlacesUtils.nodeAncestors(aNode);
I think you meant to delete this line too.
>+    // A node is removed from the view either if it has no parent or if its
>+    // root-ancestor is not the root node (in which case that's the node
>+    // for which nodeRemoved was called).
>+    let ancestors = [x for each (x in PlacesUtils.nodeAncestors(aNode))];
>+    if (ancestors.length == 0 ||
>+        ancestors[ancestors.length - 1] != this._rootNode) {
>+      throw new Error("Removed node passed to _getRowForNode");
>+    }

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Ian Neal from comment #3)
> >+++ b/suite/common/places/treeView.js
> > 
> >     let ancestors = PlacesUtils.nodeAncestors(aNode);
> I think you meant to delete this line too.

And I assume you missed to update
{
/suite/common/history/treeView.js
    * line 824 -- this._cellProperties = new WeakMap();
}
too.

http://mxr.mozilla.org/comm-central/search?string=WeakMap&case=1&find=%2Fsuite%2Fcommon%2F

Comment 5

[Frank Wein [:mcsmurf]]

Attachment: Patch 2 (not tested yet)

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 646592 [details] [diff] [review]
Patch 2 (not tested yet)

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

::: suite/common/history/treeView.js
@@ +820,5 @@
>  
>      if (val) {
>        this._result = val;
>        this._rootNode = val.root;
> +      this._cellProperties = new Map();

The other treeView.js changes should be ported to this file too.

Comment 7

[Philip Chee]

Weak Map warnings are now fatal errors.

Sat Aug 11 2012 02:11:00
Error: TypeError: cannot use the given object as a weak map key
Source file: chrome://communicator/content/places/treeView.js
Line: 1194
 ----------
Sat Aug 11 2012 02:10:56
Error: cannot use the given object as a weak map key
Source file: chrome://communicator/content/places/controller.js
Line: 1312
 ----------
Sat Aug 11 2012 02:10:53
Error: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "cannot use the given object as a weak map key" {file: "chrome://communicator/content/places/browserPlacesViews.js" line: 72}]' when calling method: [nsINavHistoryResultObserver::result]
Source file: chrome://communicator/content/places/browserPlacesViews.js
Line: 49

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #7)
> Weak Map warnings are now fatal errors.

Right, the Bookmarks Toolbar is now fully broken in recent nightlies, too. We need to fix this ASAP.

Comment 10

[Frank Wein [:mcsmurf]]

Attachment: Patch 3

This patch makes bookmarks and session restore (seems to be affected, too) working again.

Comment 11

[Ian Neal]

(In reply to Frank Wein [:mcsmurf] from comment #10)
> Created attachment 650993 [details] [diff] [review]
> Patch 3
> 
> This patch makes bookmarks and session restore (seems to be affected, too)
> working again.

The following change in the Firefox patch isn't in this one:
     if (aNode == this._rootNode)
-      throw "The root node is never visible";
+      throw new Error("The root node is never visible");

Comment 12

[Frank Wein [:mcsmurf]]

Comment on attachment 650993 [details] [diff] [review]
Patch 3

Tested this patch by doing: Creating bookmarks, moving bookmarks and deleting bookmarks (both via Bookmarks menu and bookmarks manager).
Looking at browser history, searching for something in history and deleting an entry in history.
Running mochitest-chrome in suite/, no errors.

Comment 13

[Frank Wein [:mcsmurf]]

(In reply to Ian Neal from comment #11)
> The following change in the Firefox patch isn't in this one:
>      if (aNode == this._rootNode)
> -      throw "The root node is never visible";
> +      throw new Error("The root node is never visible");

I actually wondered if the FF version is better or if this does not matter at all.

Comment 14

[Ian Neal]

(In reply to Ian Neal from comment #11)
> (In reply to Frank Wein [:mcsmurf] from comment #10)
> > Created attachment 650993 [details] [diff] [review]
> > Patch 3
> > 
> > This patch makes bookmarks and session restore (seems to be affected, too)
> > working again.
> 
> The following change in the Firefox patch isn't in this one:
>      if (aNode == this._rootNode)
> -      throw "The root node is never visible";
> +      throw new Error("The root node is never visible");
(in history's treeView.js, you've made the change in place's one)

Comment 15

[Frank Wein [:mcsmurf]]

Comment on attachment 650993 [details] [diff] [review]
Patch 3

I checked this patch in due to the severity of this bug. I included the fix from Comment 14 and checked again if everything is working.
http://hg.mozilla.org/comm-central/rev/0c05edb72d2b

Comment 16

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 SeaMonkey/2.14a1 ID:20120811112900 c-c:6a48e685f260 m-c:28aa0856c08e

The Bookmarks menu (user-defined part, after the Unsorted bookmarks) has come back, and so has the "Bookmarks → Bookmarks Toolbar" submenu (and they work). The "Unsorted Bookmarks" submenu again displays (Empty) in greyed out instead of a 2x2-pixel "submenu".

History looks normal but I didn't look at it when it was supposedly broken.

The Bookmarks Manager still works but it wasn't broken yesterday AFAICT.

I would have set VERIFIED on this bug except that it hasn't yet been set to FIXED. mcsmurf, are there other patches in progress for this bug?

Comment 17

[Tony Mechelynck [:tonymec]]

Huh? I thought I hadn't changed the Severity :-???

Comment 18

[Philip Chee]

Patch was landed to fix the bookmarks breakage with r=pending
Still needs a post-landing review and if necessary followup patches to fix any issues arising from the review.

Comment 19

[[e-mail]]

Comment on attachment 650993 [details] [diff] [review]
Patch 3

>+    let ancestors = [x for each (x in PlacesUtils.nodeAncestors(aNode))];
>+    if (ancestors.length == 0 ||
>+        ancestors[ancestors.length - 1] != this._rootNode) {
Not ideal but I'm not going to argue for a bustage fix.

Comment 20

[Philip Chee]

I guess this is fixed.
> Not ideal but I'm not going to argue for a bustage fix.
So followup patch or new bug?

Comment 21

[Frank Wein [:mcsmurf]]

Fixed for now.

Comment 23

[masinick]

I will have to see if there is another Seamonkey Nightly Build available yet; as of the most recent one I have tested within the past 24 hours, the messed up bookmarks toolbar was NOT fixed.

Comment 24

[masinick]

This bug is definitely NOT fixed!  I am running the most current Seamonkey Nightly, updated from the most recent build; I was running the most recent Firefox Nightly right before this; Firefox Nightly works, Seamonkey Nightly does not.

Iceape, the most recent stable release, DOES work, so this defect, at least the one I created - which got folded into this defect, is NOT fixed.  Please reset and correct.  What still doesn't work is that bookmarks are not visible or selectable, except from the Manage Bookmarks feature; bookmark toolbar is not correctly populated.

Comment 25

[Tony Mechelynck [:tonymec]]

(In reply to masinick from comment #24)
> This bug is definitely NOT fixed!  I am running the most current Seamonkey
> Nightly, updated from the most recent build; I was running the most recent
> Firefox Nightly right before this; Firefox Nightly works, Seamonkey Nightly
> does not.

IIUC the most recent nightly was built before the fix landed. The trunk tree is all burning red ATM. But I've been able to verify the fix in an hourly build which, alas, is not available anymore.

What is the "Build Identifier" at the bottom of your about: page? It should be a datestamp in the format yyyyMMddhhmmss. If it is strictly earlier than 20120811112900 you've got a SeaMonkey build older than the fix.

Another possibility is that you might be using an Aurora (2.13a2) or Beta (2.12) build, where this bugfix has not yet been ported. FIXED means fixed-on-trunk, which for SeaMonkey currently means the 2.14a1 codebase.

Comment 26

[masinick]

My Seamonkey Nightly build images, which I install in a directory beneath my home directory, are NIGHTLY, not Aurora or anything else.  I am well aware of the differences.  TODAY I brought the Nightly to the most current images, and most of the files within the directory have a date on them that is 08/12/12 at 23:40, so apparently that is when most of the stuff was created, regardless of what the snap image happens to be.  I'm not going to attempt to run it again until someone can positively confirm for me when the build containing the fix is actually in NIGHTLY, and THEN I will run it and confirm the fix.  I don't want to risk corrupting anything in either my bookmarks (which I use elsewhere), or for that matter, risk anything in the Email component, which has also had moments of instability over the past month.

So let me know when the NIGHTLY, not the HOURLY, build is actually downloadable using the standard updating mechanism, then I will verify it and let you know from my perspective if it is fixed to my satisfaction or not.  Fair enough?

Comment 27

[Tony Mechelynck [:tonymec]]

(In reply to masinick from comment #26)
> My Seamonkey Nightly build images, which I install in a directory beneath my
> home directory, are NIGHTLY, not Aurora or anything else.  I am well aware
> of the differences.  TODAY I brought the Nightly to the most current images,
> and most of the files within the directory have a date on them that is
> 08/12/12 at 23:40, so apparently that is when most of the stuff was created,
> regardless of what the snap image happens to be.  I'm not going to attempt
> to run it again until someone can positively confirm for me when the build
> containing the fix is actually in NIGHTLY, and THEN I will run it and
> confirm the fix.  I don't want to risk corrupting anything in either my
> bookmarks (which I use elsewhere), or for that matter, risk anything in the
> Email component, which has also had moments of instability over the past
> month.
> 
> So let me know when the NIGHTLY, not the HOURLY, build is actually
> downloadable using the standard updating mechanism, then I will verify it
> and let you know from my perspective if it is fixed to my satisfaction or
> not.  Fair enough?

I see you are as "New to Bugzilla" as your comment headers say. Please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html then please note the following:

- RESOLVED FIXED means the fix has been landed into the comm-central (or mozilla-central, as the case may be) source repository, even if nothing has yet been built from it.

- VERIFIED FIXED means that someone from QA (that's me) has installed a build made after the fix and checked that it doesn't suffer from the bug.

- If some senior developer overrides me, VERIFIED may be turned back to RESOLVED. The bug remains FIXED (see comment #21) unless someone installs a build made from a source containing the bug fix and finds the bug still there. I won't accept the datestamp on "the majority" of the files in your directory, that might be the time when you unpacked them, depending on how the unpack was done. Bug 782310 comment #0 tells me that when you reported it you were using a SeaMonkey build dated 20120811003003, which was more than 10 hours before the fix was landed.

Comment 28

[Tony Mechelynck [:tonymec]]

P.S. The build ID and the mozilla-central and comm-central changeset IDs can also be found from the seamonkey-*.txt text file sitting on the FTP server next to the .tar.bz2, .dmg, .zip or .installer.exe which you used to install the problematic build.

Comment 29

[Tony Mechelynck [:tonymec]]

Masinick: The next nightly (when there is one) will have the fix. In the meantime, I see some new hourlies have been built. You can, if you want, download one from:
http://ftp.mozilla.org/pub/mozilla.org/seamonkey/tinderbox-builds/comm-central-trunk-linux64/1344896520/ for 64-bit Linux, or
http://ftp.mozilla.org/pub/mozilla.org/seamonkey/tinderbox-builds/comm-central-trunk-linux/1344896520/ for 32-bit Linux.

Each of these will be removed in 24 hours or so, so if you want them, act fast. Then you can wait more calmly for a nightly .tar.gz with a date of today or later on the server.

Comment 30

[masinick]

Thank you Tony!  I will give that one a try, and then I'll keep updating from there with the next Nightly Build.  I should, assuming this particular build works, be able to verify it in relatively short order.  I'll try to do that within the next hour!  Appreciate it!

Brian

Comment 31

[masinick]

Hi Tony!  Build identifier: 20120813152200 does the job, so I would expect that the next Nightly Build will also get it done!  Thank you!  From my perspective, this build fixes the problem, so as this one percolates up, it solves the problem.  Again, thanks for taking the extra time to show me these details.  I'll keep regularly using these builds to make sure that they are great quality!

Brian Masinick
Regular Seamonkey Nightly and Firefox Nightly user and tester

Comment 32

[Tony Mechelynck [:tonymec]]

In reply to comment #31: I'm happy to see that you got your problem solved, but I'm not sure that this hourly build will auto-update. You can get nightly builds from http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk/ — just be sure to check the date of the .tar.gz on the server: you wouldn't want to install an older build over the working one. ;-)

Comment 33

[Tony Mechelynck [:tonymec]]

s/.tar.gz/.tar.bz2/

Comment 34

[masinick]

Thanks again Tony.  I will see if the Tinderbox build will auto update.  If not, I'll grab the build from the latest-comm-central trunk/.  It will be interesting to see whether the Tinderbox builds have the auto update feature or not...

Comment 35

[masinick]

Yeah, looks like I'll have to go to the trunk to get back on the Nightly track...

Comment 36

[Tony Mechelynck [:tonymec]]

Developers: According to discussion on IRC, and unlike what happened on trunk just before the fix landed, on Aurora the "visible" symptoms (missing menus etc.) are absent, only the Error Console errors are seen.

Comment 37

[masinick]

OK, I am here now with Build identifier: 20120814003006, and like the Build identifier: 20120813152200 that does the job, the fix has in fact carried forward to 20120814003006, as we would expect.  So thanks everyone.  From my perspective, this is a done deal; works up through Nightly now, so I am happy.

Comment 38

[Philip Chee]

Comment on attachment 650993 [details] [diff] [review]
Patch 3

[Approval Request Comment]
Regression caused by (bug #): Bug 730340

User impact if declined: WeakMap keys can disappear at any time, that means we may consider a node nonexisting when it really exists. The UI consequences are unknown, likely bookmark/history UI pieces may stop working.

Testing completed (on m-c, etc.): m-c, c-c

Risk to taking this patch (and alternatives if risky): Should not be much risky, we are replacing wekmaps with maps, the worse thing would be a leak, but didn't show any in tests.

String or UUID changes made by this patch: none

The base bug Bug 760940 was back ported to aurora and beta.

Comment 40

[[e-mail]]

(In reply to Philip Chee from comment #20)
> I guess this is fixed.
> > Not ideal but I'm not going to argue for a bustage fix.
> So followup patch or new bug?
Not sure. Best I can do is:
for (let ancestor = aNode.parent; ancestor != this._rootNode;
     ancestor = ancestor.parent)
  if (!ancestor)
    throw new Error("Removed node passed to _getRowForNode");
  else if (!ancestor.containerOpen)
    throw new Error("Invisible node passed to _getRowForNode");

Comment 41

[Justin Wood (:Callek)]

http://hg.mozilla.org/releases/comm-beta/rev/4519d9baf3f4

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to Justin Wood (:Callek) from comment #41)
> http://hg.mozilla.org/releases/comm-beta/rev/4519d9baf3f4

Callek, you pushed the patch instead of the changeset once again :-/
I assume we can live with that though :-|

Comment 43

[Philip Chee]

> Callek, you pushed the patch instead of the changeset once again :-/
What's the difference between pushing the patch and pushing the changeset?

Comment 44

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #43)
> > Callek, you pushed the patch instead of the changeset once again :-/
> What's the difference between pushing the patch and pushing the changeset?

The changeset may (as in this case) contain changes made between the creation of the patch and the commit to the upstream repository.

To push a changeset to a different branch you could use hg transplant, but I use the following instead so I can use mq:

function qtransplant() {
  # cf. http://weblogs.mozillazine.org/gerv/archives/2010/06/hg_transplant.html
  (cd ../comm-central && hg export "$1") | hg qimport - --name "$2"
}
function qed() {
  vi .hg/patches/$1
}
function QT() {
  qtransplant "$1" "$2"
  qed "$2"
}

To be used like this from the target repository's checkout directory ("a" will be the mq patch name and could be anything; I usually use a, a2 etc. for Aurora and b, b2 etc. for Beta for brevity):

QT http://hg.mozilla.org/comm-central/rev/0c05edb72d2b a

Note: Serge usually explicitly names the changeset(s) to push in the whiteboard after branch approval is granted. In this case he didn't have the chance to do so since Callek granted and pushed at the same time.

Comment 45

[Philip Chee]

http://hg.mozilla.org/releases/comm-aurora/rev/9e694192b1e1

Comment 46

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #42)
> (In reply to Justin Wood (:Callek) from comment #41)
> > http://hg.mozilla.org/releases/comm-beta/rev/4519d9baf3f4
> 
> Callek, you pushed the patch instead of the changeset once again :-/
> I assume we can live with that though :-|

(In reply to Philip Chee from comment #45)
> http://hg.mozilla.org/releases/comm-aurora/rev/9e694192b1e1

And so did you :-/

Comment 47

[Philip Chee]

Dunno. All I did was to use hg transplant -s ../comm-beta/ <REV> since that had the right commit comment.

Comment 48

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #47)
> Dunno. All I did was to use hg transplant -s ../comm-beta/ <REV> since that
> had the right commit comment.

Yes, comm-aurora had the right comment, but the "wrong" content" ;-> Anyway.

Comment 49

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #48)
> Yes, comm-aurora had the right comment, but the "wrong" content" ;-> Anyway.

Arf, s/comm-aurora/comm-beta/...

Comment 50

[Philip Chee]

>> -      throw "The root node is never visible";
>> +      throw new Error("The root node is never visible");

> I actually wondered if the FF version is better or if this does not matter at all.
Actually you should use throw Components.Exception()
<https://developer.mozilla.org/en-US/docs/Components.Exception>