Closed Bug 777005 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

(seamonkey2.12 fixed, seamonkey2.13 fixed, seamonkey2.14 verified)

VERIFIED FIXED
seamonkey2.14
Tracking Status
seamonkey2.12 --- fixed
seamonkey2.13 --- fixed
seamonkey2.14 --- verified

People

(Reporter: philip.chee, Assigned: mcsmurf)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
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
>
Attached patch Patch (not tested yet) (obsolete) — Splinter Review
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");
>+    }
(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
Flags: in-testsuite-
Attached patch Patch 2 (not tested yet) (obsolete) — Splinter Review
Attachment #645471 - Attachment is obsolete: true
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.
Attachment #646592 - Flags: feedback-
Attachment #645471 - Flags: feedback-
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
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
Severity: normal → critical
Duplicate of this bug: 781791
(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.
Severity: critical → blocker
Attached patch Patch 3Splinter Review
This patch makes bookmarks and session restore (seems to be affected, too) working again.
Attachment #646592 - Attachment is obsolete: true
(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 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.
Attachment #650993 - Flags: review?(neil)
(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.
(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 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
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?
Severity: blocker → critical
Huh? I thought I hadn't changed the Severity :-???
Severity: critical → blocker
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.
Whiteboard: [leave open]
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.
Attachment #650993 - Flags: review?(neil) → review+
I guess this is fixed.
> Not ideal but I'm not going to argue for a bustage fix.
So followup patch or new bug?
Fixed for now.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Duplicate of this bug: 782310
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.
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.
(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.
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?
(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.
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.
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.
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
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
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. ;-)
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...
Yeah, looks like I'll have to go to the trunk to get back on the Nightly track...
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.
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 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.
Attachment #650993 - Flags: approval-comm-beta?
Attachment #650993 - Flags: approval-comm-aurora?
Priority: -- → P1
Target Milestone: --- → seamonkey2.14
Duplicate of this bug: 782880
(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");
Attachment #650993 - Flags: approval-comm-beta?
Attachment #650993 - Flags: approval-comm-beta+
Attachment #650993 - Flags: approval-comm-aurora?
Attachment #650993 - Flags: approval-comm-aurora+
(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 :-|
> Callek, you pushed the patch instead of the changeset once again :-/
What's the difference between pushing the patch and pushing the changeset?
(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.
(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 :-/
Dunno. All I did was to use hg transplant -s ../comm-beta/ <REV> since that had the right commit comment.
(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.
(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/...
>> -      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>
You need to log in before you can comment on or make changes to this bug.