Batch insertion for test_cookies_async_failure.js

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: Cookies
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: junior, Assigned: junior)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 months ago
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.
(Assignee)

Comment 1

5 months ago
Created attachment 8874716 [details] [diff] [review]
Part1: removeDupTest, v1

Per bug 1158387 comment 16, we should remove the dup code.
Attachment #8874716 - Flags: review?(mak77)
(Assignee)

Comment 2

5 months ago
Created attachment 8874717 [details] [diff] [review]
Part2: expose begin/commit transaction v1

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)

Updated

5 months ago
Attachment #8874716 - Flags: review?(mak77) → review+

Comment 3

5 months ago
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.
(Assignee)

Comment 4

5 months ago
Created attachment 8874808 [details] [diff] [review]
Part2: expose runInBatchMode - v2

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)

Comment 5

5 months ago
personally I think runInBatchMode was an horrible name and runInTransaction would be better, but that's just my 2 cents.
(Assignee)

Comment 6

5 months ago
Created attachment 8874821 [details] [diff] [review]
Part2: expose runInTransaction - v3
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 7

4 months ago
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+

Comment 9

4 months ago
xpcom black magic (basically the function decorator on the interface).
(Assignee)

Comment 10

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a7eb9b093785d6bbf51652363d78bf9282e7b9a
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 11

4 months ago
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

Comment 12

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ae4d9b0f74c
https://hg.mozilla.org/mozilla-central/rev/94df53e8cc7e
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.