Closed Bug 691512 (addonMgrContention) Opened 8 years ago Closed 8 years ago

Don't mixup synchronous and asynchronous createStatements in AddonRepository.jsm

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

If you use the same connection for synchronous and asyncrhonous APIs and some async stuff takes some time to run, there is risk the synchronous API (like createStatement) may block on it.

This patch fixed AddonRepository, looks like there are other dangerous usages in XPIProvider.jsm but those may need more attention, you may want different statements on different connections.

PS: I'm not sure I have the time to look into the remaining and more complex cases shortly, but I'm happy to review the changes if you wish.
Attachment #564325 - Flags: review?(dtownsend)
(In reply to Marco Bonardo [:mak] from comment #0)
> Created attachment 564325 [details] [diff] [review] [diff] [details] [review]
> AddonRepository patch v1.0
> 
> If you use the same connection for synchronous and asyncrhonous APIs and
> some async stuff takes some time to run, there is risk the synchronous API
> (like createStatement) may block on it.

As far as I can see AddonRepository only ever executes statements asynchronously and all the patch does is change us to explicitely creating them as async statements. This is clearly a good thing but I wonder what the risk is here, am I missing something?

> This patch fixed AddonRepository, looks like there are other dangerous
> usages in XPIProvider.jsm but those may need more attention, you may want
> different statements on different connections.

What I wouldn't give for the time to be able to make XPIProvider.jsm purely async :(
Attachment #564325 - Flags: review?(dtownsend) → review+
Blocks: StorageJank
I hate myself for having lost this bug from my radar. Will try to unbitrot and push it today and will file a separate bug for XPIProvider.jsm

(In reply to Dave Townsend (:Mossop) from comment #1)
> As far as I can see AddonRepository only ever executes statements
> asynchronously and all the patch does is change us to explicitely creating
> them as async statements. This is clearly a good thing but I wonder what the
> risk is here, am I missing something?

For a good explanation see https://bugzilla.mozilla.org/show_bug.cgi?id=686025#c21
Plus, over the thread contention issues, createrStatement actually creates 2 statement objects (one for each thread) while you just need one since your statement runs only asynchronously.

> What I wouldn't give for the time to be able to make XPIProvider.jsm purely
> async :(

Heh, will file a bug on that (provided there isn't one already?)
Assignee: nobody → mak77
Alias: addonMgrContention
(In reply to Marco Bonardo [:mak] from comment #2)
> I hate myself for having lost this bug from my radar. Will try to unbitrot
> and push it today and will file a separate bug for XPIProvider.jsm
> 
> (In reply to Dave Townsend (:Mossop) from comment #1)
> > As far as I can see AddonRepository only ever executes statements
> > asynchronously and all the patch does is change us to explicitely creating
> > them as async statements. This is clearly a good thing but I wonder what the
> > risk is here, am I missing something?
> 
> For a good explanation see
> https://bugzilla.mozilla.org/show_bug.cgi?id=686025#c21
> Plus, over the thread contention issues, createrStatement actually creates 2
> statement objects (one for each thread) while you just need one since your
> statement runs only asynchronously.

So reading that it is not mixing synchronous and asynchronous statements that causes problems, it is just using createStatement at all. That makes more sense to me.
Summary: Addon manager mixed use of synchronous and asynchronous queries may cause contention → Don't mixup synchronous and asynchronous statements in AddonRepository.jsm
Summary: Don't mixup synchronous and asynchronous statements in AddonRepository.jsm → Don't mixup synchronous and asynchronous createStatements in AddonRepository.jsm
https://hg.mozilla.org/integration/mozilla-inbound/rev/91c7824254cb
Flags: in-testsuite-
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/91c7824254cb
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(In reply to Dave Townsend (:Mossop) from comment #3)
> So reading that it is not mixing synchronous and asynchronous statements
> that causes problems, it is just using createStatement at all. That makes
> more sense to me.
Note: both cases cause problems :)
You need to log in before you can comment on or make changes to this bug.