382073 - Bookmarks Service batch update API is a footgun

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Robert Sayre]

See the interaction here:

https://bugzilla.mozilla.org/show_bug.cgi?id=379729#c17

This should not be exposed to extension authors. I don't know if it means we need to take it out of the IDL or just discourage it in favor of a FUEL API, but we can't allow random code access to that.

Comment 1

[Mike Shaver (:shaver emeritus)]

Would extension authors not need it in the same cases as we need it here, or similar ones such as doing a large sync with a web service?  Extension authors can do a ton of stuff to hang the browser, and while I totally approve of wrapping sharp-edged behaviour in easy-grip API helpers, I think that "not allowing random code access to that" is a non-starter.

Comment 2

[Mike Shaver (:shaver emeritus)]

Possibly a useful pattern to expose in FUEL or some other wrapper, to remove the vast majority of this bug habitat:

https://bugzilla.mozilla.org/attachment.cgi?id=220626

Comment 3

[Robert Sayre]

> I think that "not allowing random code access to that" is a non-starter.

Why? Because we can't or because don't want to? If it's the latter, I strongly disagree.

Comment 4

[Mike Shaver (:shaver emeritus)]

The latter.  Extension authors don't have lesser needs than the Firefox developers, especially since we're expecting them to build the more advanced and experimental features atop the Places APIs (versus the relatively conservative set that we're shipping with).  But I'm not sure why you would "strongly disagree", given that one of your _own_ proposals (providing a helpful wrapper and discouraging direct batching calls) would still allow random code to access the batching API, it would just reduce the need to use it in direct/dangerous fashion.

(There are many ways for extension authors to lock up the app, even without resorting to C++; that is one cost of having such deep access to the workings of Firefox.)

There are alternative APIs that might be less error prone here, such as creating a transaction object that ends the batch on destruction, as well as via an explicit .endBatch call.  We could then warn about cases in which .endBatch wasn't called and the object had to bail the developer out.  That's unfortunately a fair bit of API juggling, and I think that providing a utility wrapper via FUEL is likely the best course here. (It could possibly simulate the batching when atop the Fx2 RDF API as well, in time.)

Comment 5

[Robert Sayre]

(In reply to comment #4)
> The latter.  Extension authors don't have lesser needs than the Firefox
> developers, 

I understand that, but I care more about our user's data than extension developers' needs. If we leave this as-is, users will lose data. We can get away with it because we can QA our uses of it for months.

I'm not advocating preventing extension authors from using a batching API. I just don't want this particular pair of functions exposed to code we didn't write. We should probably figure out if there's a way for us to avoid using them as well.

Comment 6

[Robert Sayre]

maybe we just need this 

function withBatching(f) {
  bms.beginBatching();
  try {
    f();
  } finally {
    bms.endBatching();
  }
}

Comment 7

[Asaf Romano (gone)]

Taking, FUEL should not be our way to hide sub-optimal interfaces, my plan is to do a C++- version of comment 6.

Comment 8

[Mike Shaver (:shaver emeritus)]

That's actually the whole point of FUEL -- if our interfaces weren't painful, we wouldn't really need it. :/

Why do it in C++?

Comment 9

[Asaf Romano (gone)]

(In reply to comment #8)
> That's actually the whole point of FUEL -- if our interfaces weren't painful,
> we wouldn't really need it. :/

I tend to agree that FUEL isn't the right solution for non-frozen stuff. 

> Why do it in C++?

Because nsNavBookmarks is written in that language. 

Comment 10

[Mike Shaver (:shaver emeritus)]

(In reply to comment #9)
> (In reply to comment #8)
> > That's actually the whole point of FUEL -- if our interfaces weren't painful,
> > we wouldn't really need it. :/
> 
> I tend to agree that FUEL isn't the right solution for non-frozen stuff. 

Do you mean that FUEL should only wrap frozen stuff?  If so, I very much disagree.   One of the purposes of FUEL is to isolate common tasks from the underlying mechanics, such that add-ons can perform simple bookmark manipulation (f.e.) compatibly between Fx2 and Fx3.

We fully intend to wrap a bunch of a non-frozen stuff, if only because you can't really do much that's useful with only frozen APIs.

Comment 11

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

(In reply to comment #10)
> Do you mean that FUEL should only wrap frozen stuff?

I think what Mano means is that the existence of FUEL shouldn't stop us from fixing sub-optimal unfrozen interfaces to make them easier to use. If we can change the interface to make it easier to use, we should, especially if we can do it as part of a larger redesign that is going to break compatibility anyways (e.g. Places).

Comment 12

[Mike Shaver (:shaver emeritus)]

(In reply to comment #11)
> (In reply to comment #10)
> > Do you mean that FUEL should only wrap frozen stuff?
> 
> I think what Mano means is that the existence of FUEL shouldn't stop us from
> fixing sub-optimal unfrozen interfaces to make them easier to use.

Oh, yes, I agree totally.  I was just looking at it as an opportunity to take some work off the Fx2 guys and make mfinkle or jresig do it. :)

Comment 13

[Asaf Romano (gone)]

Comment 11 is mine.

Comment 14

[Mike Shaver (:shaver emeritus)]

Aroo?

Comment 15

[Asaf Romano (gone)]

Attachment: patch

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> Created an attachment (id=266534) [details]
> patch
> 

this patch does not apply cleanly. can you update?

Comment 17

[Asaf Romano (gone)]

Attachment: patch

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 266534 [details] [diff] [review]
patch



>+  // called back from handleResult
>+  runBatched: function LLL_runBatched(aClosure) {

nit: please add a comment about the interaction going on here. especially b/c handleResult just says "see the idl"

everything else looked ok, except for the autolock, which i'm not familiar with. please have seth or someone else do a review of that part. also, would be good to have sayrer take a look at the livemark service changes.

r=me, with the above comments, and assuming that you've tested this (see comment #16).

Comment 19

[Robert Sayre]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > Why do it in C++?
> 
> Because nsNavBookmarks is written in that language. 
> 

We can't declare that a C++ function is usable by a single JS file, afaik. Thus, Mano's solution seems like the best trade-off to me. Please consider renaming the "closure" to "userData". JS written in the style of comment #6 would never have such a parameter, and I would like to pretend we live in that world. :)

Livemarks changes look fine.

Comment 20

[(not reading, please use [e-mail] instead)]

As for why this would be good for A5, from #places:

<Mano> dietrich: it seems to fix a shutdown crash+dataloss issue from seth's batch patch.
<dietrich> Mano: right, sayrer mentioned something about that last night
<sspitzerMsgMe> Mano:  yoinks!  do we have a bug / stack on the crash from my batch patch?
<Mano> dietrich: thus the lock
<Mano> sspitzerMsgMe: not really, and i'm not sure whether your patch alone cause that in a way which is easy to reproduce
<Mano> sspitzerMsgMe: basically, begin/end-batch was not thread-safe
<Mano> and given the way the livemark service works we end up shutting down without writing the changes to the DB
<Mano> (thus the new lock)
<Mano> now, xpconnect might have hidden that with its own locks
<Mano> but i don't know that for sure.

Comment 21

[Robert Sayre]

Comment on attachment 266675 [details] [diff] [review]
patch

Seems to me that you don't need mImportChannel if you can call GetChannel on the parser. Or am I missing something?

Also, it seems prudent to be paranoid and ensure that mLock has been initialized.

Comment 22

[Robert Sayre]

Comment on attachment 266675 [details] [diff] [review]
patch

>+  nsAutoLock lock(mLock);
>+  BeginUpdateBatch();
>+  nsresult rv = aCallback->RunBatched(aClosure);
>+  if (NS_FAILED(rv)) {
>+    EndUpdateBatch();
>+    return rv;
>+  }
>+  EndUpdateBatch();

Probably a result of editing something out of that if-block.

EndUpdateBatch();
NS_ENSURE_SUCCESS(rv, rv);

Comment 23

[Asaf Romano (gone)]

Attachment: patch

Comment 24

[Robert Sayre]

Comment on attachment 266829 [details] [diff] [review]
patch

sicking, please check the parser stuff down near the bottom in the context of the locking added to RunInBatchMode

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 266829 [details] [diff] [review]
patch

mconnor asked me to review this as well as sayrer, so I read through it, and everything looks fine.

Comment 26

[Asaf Romano (gone)]

mozilla/browser/components/places/content/controller.js 1.158
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.22
mozilla/browser/components/places/src/nsPlacesImportExportService.h 1.8
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.45
mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.63
mozilla/toolkit/components/places/src/nsLivemarkService.js 1.16
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.100
mozilla/toolkit/components/places/src/nsNavBookmarks.h 1.42
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.130
mozilla/toolkit/components/places/src/nsNavHistory.h 1.80

Comment 27

[Shawn Wilsher :sdwilsh]

Can someone explain to me the use of the lock?  None of these interfaces are marked as threadsafe, so they shouldn't be used on other threads.  So, are we just locking on the main thread?  What for?

Comment 28

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv