Closed Bug 1370454 Opened 3 years ago Closed 3 years ago

Batch insertion for test_cookies_async_failure.js

Categories

(Core :: Networking: Cookies, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: junior, Assigned: junior)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 2 obsolete files)

Per bug 1158387 comment 17, we should use a batch insertion to avoid multiple transactions, which cause a time-out such as bug 1158387 comment 14.
Per bug 1158387 comment 16, we should remove the dup code.
Attachment #8874716 - Flags: review?(mak77)
The patch expose begin/commit transaction. We could put thousands of operations into one transaction in order to avoid jank.
Attachment #8874717 - Flags: review?(mak77)
Attachment #8874717 - Flags: review?(jduell.mcbugs)
Attachment #8874716 - Flags: review?(mak77) → review+
Comment on attachment 8874717 [details] [diff] [review]
Part2: expose begin/commit transaction v1

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

::: netwerk/cookie/nsICookieService.idl
@@ +202,5 @@
> +   * Each call to beginTransaction must have a matching commitTransaction. Do
> +   * not nest transactions!
> +   */
> +   void beginTransaction();
> +   void commitTransaction();

It's not my call to do, since I'm not even a peer of this component, but what about instead exposing a runInTransaction(callback) API, that starts a transaction, runs the callback, and commits or rollbacks the transaction based on whether func returned a succeeded rv (you need to define a [scriptable, function, ...] interface for that to work, we have something similar in history, see runInBatchMode and nsINavHistoryBatchCallback).
It won't work in an async fashion, but you are always wrapping synchronous code here.

Otherwise, this needs a big warning about never leaving transactions open, and likely a roolbackTransaction() method too (to use in try/catch). Luckily without xpcom add-ons this will be safer, but it's still a footgun for internal code.
Thanks for your suggestion, :mak.
Attachment #8874717 - Attachment is obsolete: true
Attachment #8874717 - Flags: review?(mak77)
Attachment #8874717 - Flags: review?(jduell.mcbugs)
Attachment #8874808 - Flags: review?(jduell.mcbugs)
Attachment #8874808 - Flags: feedback?(mak77)
personally I think runInBatchMode was an horrible name and runInTransaction would be better, but that's just my 2 cents.
Attachment #8874808 - Attachment is obsolete: true
Attachment #8874808 - Flags: review?(jduell.mcbugs)
Attachment #8874808 - Flags: feedback?(mak77)
Attachment #8874821 - Flags: review?(jduell.mcbugs)
Attachment #8874821 - Flags: feedback?(mak77)
Comment on attachment 8874821 [details] [diff] [review]
Part2: expose runInTransaction - v3

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

LGTM
Attachment #8874821 - Flags: feedback?(mak77) → feedback+
Comment on attachment 8874821 [details] [diff] [review]
Part2: expose runInTransaction - v3

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

::: extensions/cookie/test/unit/test_cookies_async_failure.js
@@ +210,5 @@
>  function* run_test_2(generator)
>  {
>    // Load the profile and populate it.
>    do_load_profile();
> +  Services.cookies.runInTransaction(_=>{

I'm not sure how this JS provides a nsICookieTransactionCallback object with a callback() method.  But if it works, I'll trust you.
Attachment #8874821 - Flags: review?(jduell.mcbugs) → review+
xpcom black magic (basically the function decorator on the interface).
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae4d9b0f74c
Part1: remove dup test in test_cookie_async_failure.js, r=mak
https://hg.mozilla.org/integration/mozilla-inbound/rev/94df53e8cc7e
Part2: Expose runInTransaction in nsICookieService.idl, r=jduell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2ae4d9b0f74c
https://hg.mozilla.org/mozilla-central/rev/94df53e8cc7e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.