636917 - Livemark only added to _livemarks array after anno has been added

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Richard Newman [:rnewman]]

This means that handlers for livemark annotations get the wrong answer from nodeIsLivemarkContainer when the livemark service has been loaded. Uh oh.

mak suggested "in the new bug add a note that we should check current views code since onItemAdded livemarks live update could fail".

This was discovered in the course of fixing Bug 387138.

Comment 1

[Marco Bonardo [:mak]]

this one wants a test.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I made this along the weekend but forgot to attach it.
It's a pretty much safe change, with a dedicated test, since isLivemark is used both by internal code and by Sync, its reliability should be good.
Due to the above, I would appreciate approval too.

Comment 3

[Marco Bonardo [:mak]]

This is actually a regression we made in 3.6

Comment 4

[Marco Bonardo [:mak]]

Attachment: real patch v1.0

I attached random stuff :(

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 516115 [details] [diff] [review]
real patch v1.0

bah, I should cleanup my patches queue sometimes...

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.0

ok, this should be the correct one :p

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Actually, we are also not respecting the idl in get/setFeedURI, should throw if the id is not a livemark... This fixes that, so the check in setFeedURI is not needed (you can't set a feeduri to a folder to transform it to a livemark)

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 516227 [details] [diff] [review]
patch v1.1


>   _createFolder:
>   function LS__createFolder(aParentId, aName, aSiteURI, aFeedURI, aIndex) {
>     var folderId = bms.createFolder(aParentId, aName, aIndex);
>     bms.setFolderReadonly(folderId, true);
> 
>     // Add an annotation to map the folder id to the livemark feed URI
>+    this._lastCreatedLivemarkFolderId = folderId;
>     ans.setItemAnnotation(folderId, LMANNO_FEEDURI, aFeedURI.spec,
>                           0, ans.EXPIRE_NEVER);

also add a comment about why storing the id.

>     try {
>       this._getLivemarkIndex(aFolderId);
>       return true;
>     }
>     catch (ex) {}
>+    // There is an edge case here, if a AnnotationChanged notification asks for
>+    // isLivemark and the livemark is currently being added, it is not yet in
>+    // the _livemarks array.  In such a case go the slow path.
>+    if (this._lastCreatedLivemarkFolderId === aFolderId)
>+      return ans.itemHasAnnotation(aFolderId, LMANNO_FEEDURI);
>     return false;
>   },

why here instead of in _getLivemarkIndex?


>   getFeedURI: function LS_getFeedURI(aFolderId) {
>+    this._ensureLivemark(aFolderId);
>+

what about side-effects in other code that doesn't expect this to throw?

(and same for setFeedURI)

i'm worried about making this change so late in the cycle.

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> >     try {
> >       this._getLivemarkIndex(aFolderId);
> >       return true;
> >     }
> >     catch (ex) {}
> >+    // There is an edge case here, if a AnnotationChanged notification asks for
> >+    // isLivemark and the livemark is currently being added, it is not yet in
> >+    // the _livemarks array.  In such a case go the slow path.
> >+    if (this._lastCreatedLivemarkFolderId === aFolderId)
> >+      return ans.itemHasAnnotation(aFolderId, LMANNO_FEEDURI);
> >     return false;
> >   },
> 
> why here instead of in _getLivemarkIndex?

the livemark is not yet in the _livemarks array. it cannot have a index.

> >   getFeedURI: function LS_getFeedURI(aFolderId) {
> >+    this._ensureLivemark(aFolderId);
> >+
> 
> what about side-effects in other code that doesn't expect this to throw?
> 
> (and same for setFeedURI)
> 
> i'm worried about making this change so late in the cycle.

the idl says we should throw, so I'm not worried by code that expect it to not throw.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> > >   getFeedURI: function LS_getFeedURI(aFolderId) {
> > >+    this._ensureLivemark(aFolderId);
> > >+
> > 
> > what about side-effects in other code that doesn't expect this to throw?
> > 
> > (and same for setFeedURI)
> > 
> > i'm worried about making this change so late in the cycle.
> 
> the idl says we should throw, so I'm not worried by code that expect it to not
> throw.

I'm referring to internal code that doesn't handle an exception when calling this, causing other fallout (regressions) because of that.

This change is not at all related to this bug. Please remove it from this patch and we can discuss it in a new bug.

Comment 11

[Marco Bonardo [:mak]]

ok, fine for me!

Comment 12

[Marco Bonardo [:mak]]

Attachment: patch v2.0

reduced patch, splitted the remaining part to bug 638412

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 516564 [details] [diff] [review]
patch v2.0

r=me, thanks!

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 516564 [details] [diff] [review]
patch v2.0

Asking approval for this small fix.
Handles an edge case where isLivemark returns false when it should be true. it is important because isLivemark is used in most of our UI and now in Sync.
The fix is pretty much trivial, and comes with a dedicated test.

Comment 15

[Dietrich Ayala (:dietrich)]

Can you push to try, and re-request if it's green?

Comment 16

[Marco Bonardo [:mak]]

Linux just completed, others running
http://tbpl.mozilla.org/?tree=MozillaTry&rev=ab31bfa47738

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 516564 [details] [diff] [review]
patch v2.0

retrying with green try results!

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 516564 [details] [diff] [review]
patch v2.0

Too late, sadly.

Comment 19

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/e4a1143ddbdf

Comment 20

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e4a1143ddbdf