995755 - adding new bookmarks is broken

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[kevink9876543]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 SeaMonkey/2.28a1 (Beta/Release)
Build ID: 20140413093357

Steps to reproduce:

(from clean profile)
start SeaMonkey, then try to bookmark any page from either the address bar or Bookmarks menu -> Bookmark this page.


Actual results:

Nothing.  Possibly related messages in the Error Console:

(on startup)
Error: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIURI.equals]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://communicator/content/bookmarks/browser-places.js :: PSB_updateState/this._pendingStmt< :: line 730"  data: no]
Source File: chrome://communicator/content/bookmarks/browser-places.js
Line: 730

(when using bookmarks menu)
Error: SetCharsetForURI is deprecated and will be removed in the next version.

Works as expected: c-c rev c6e5d3661bcc / m-c rev 25aeb2bc79f2
Fails: c-c rev c27f7537bc1b / m-c rev 83ae54e18689


Expected results:

The page should have been bookmarked.

Comment 1

[therube]

Note that Ctrl+D (File Bookmark) & a right-click context-menu (Bookmark This Page) continue to work.

Comment 2

[kevink9876543]

Broken by some change on m-c.  I'll try and see if I can find the exact changeset causing this.

Comment 3

[kevink9876543]

m-c rev f047476811f7 : works
m-c rev 7292ef816dc8 : doesn't work

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

mozilla-central changeset 7292ef816dc8 / Bug 989083 introduced an extra aScope parameter to PlacesUtils.asyncGetBookmarkIds. We need to do the same (simple two line change). Voluntaries (unfortunately I have very little time myself right now)?

[Note that the above is only from a quick check; no guarantee this is all that need to be done here, but quite likely. Also check with Neil whether that new "=>" syntax is needed and/or welcome in suite code.]

Comment 5

[kevink9876543]

Since no one has done anything about this yet and the affected files are all JS, decided to look at it myself.  The following changeset fixed it for me:

--- a/suite/common/bookmarks/browser-places.js	Wed Apr 02 00:30:50 2014 +0200
+++ b/suite/common/bookmarks/browser-places.js	Tue Apr 22 18:52:45 2014 -0400
@@ -725,7 +725,7 @@
       delete this._pendingStmt;
     }
 
-    this._pendingStmt = PlacesUtils.asyncGetBookmarkIds(this._uri, function (aItemIds, aURI) {
+    this._pendingStmt = PlacesUtils.asyncGetBookmarkIds(this._uri, (aItemIds, aURI) => {
       // Safety check that the bookmarked URI equals the tracked one.
       if (!aURI.equals(this._uri)) {
         Components.utils.reportError("PlacesStarButton did not receive current URI");
@@ -751,7 +751,7 @@
       }
 
       delete this._pendingStmt;
-    }, this);
+    });
   },
 
   _updateStateInternal: function PSB__updateStateInternal()



(It was not enough just to delete the last parameter passed to asyncGetBookmarkIds - apparently, the => syntax really does make a difference here.  I'm not happy coding with a syntax I don't understand and that doesn't seem to be documented on MDN...
Could someone please explain that syntax or write documentation for it on MDN?)

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

(In reply to kevink9876543 from comment #5)
> The following changeset fixed it for me:

Would you like to try and provide a ready patch and request review? [If not, thanks anyway so far!]

> (It was not enough just to delete the last parameter passed to
> asyncGetBookmarkIds - apparently, the => syntax really does make a
> difference here.

Yes it changes the meaning of "this". That can be worked around though (see examples on the second page linked below).

> I'm not happy coding with a syntax I don't understand

Who is? ;-)

> and that doesn't seem to be documented on MDN...

It is documented; it's just that search engines generally suck at finding non-word strings.

> Could someone please explain that syntax or write documentation for it on
> MDN?)

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_and_function_scope#The_arrow_function_expression_%28.3D%3E%29

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/arrow_functions

Comment 7

[kevink9876543]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #6)
> Would you like to try and provide a ready patch and request review? [If not,
> thanks anyway so far!]
Well, I'm only an intermediate level JS programmer, and since I haven't done this before, I will probably need help with everything other than actual JS coding and addressing review comments.  If you or someone else could help me with the non-JS-coding part, and if my JS coding ability is sufficient, I'd be willing to try.  Otherwise, it would be better for someone else to take this.

> It is documented; it's just that search engines generally suck at finding
> non-word strings.
Yeah, I was searching for "=>" and getting zero results.  Thanks for pointing me to the relevant docs, that clears that up (and now I think I could re-write that patch without the arrow syntax, if needed).

Comment 8

[Philip Chee]

You could use .bind(this) if you don't want to use the fat arrow syntax.

https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Function/bind

Comment 9

[kevink9876543]

Attachment: Above changeset in review-ready format

OK, I've read some docs and it doesn't look like it's too hard to generate a review-ready patch, so here it is.
Seems like the right person to request review from would indeed be "Neil" - would that be this Neil
https://bugzilla.mozilla.org/show_bug.cgi?id=998724#c3
?
Just want to make sure before setting the flag...

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #8)
> You could use .bind(this) if you don't want to use the fat arrow syntax.

And I think that's what we should do if it works, given that we don't use the new syntax anywhere else yet and this is merely a fix rather than lots of new code. But let's have the reviewer decide upon that.

(In reply to kevink9876543 from comment #9)
> OK, I've read some docs and it doesn't look like it's too hard to generate a
> review-ready patch, so here it is.

:-)

> Seems like the right person to request review from would indeed be "Neil" -
> would that be this Neil
> https://bugzilla.mozilla.org/show_bug.cgi?id=998724#c3
> ?

Yes.

Comment 11

[[e-mail]]

Actually => used to do a .bind but this disabled the JIT for some reason so it was recently rewritten and is now the most efficient available* choice.

* Other ways of implementing callbacks exist which may or may not be more efficient than fat arrow functions depending on the use case.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/80184b569ecf

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 8411454 [details] [diff] [review]
Above changeset in review-ready format

[Approval Request Comment]
Regression caused by (bug #): bug 989083
User impact if declined: adding new bookmarks is broken
Testing completed (on m-c, etc.): landed on c-c
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

Comment 14

[kevink9876543]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #13)
> User impact if declined: adding new bookmarks is broken
It's worse than that - in fact, the entire StarUI is dead without this patch (thus my commit message).  Users wouldn't be able to edit or remove existing bookmarks using the address bar either.

Comment 15

[Philip Chee]

Comment on attachment 8411454 [details] [diff] [review]
Above changeset in review-ready format

a=me for comm-aurora

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/comm-aurora/rev/06046995a33d