691512 - (addonMgrContention) Don't mixup synchronous and asynchronous createStatements in AddonRepository.jsm

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Marco Bonardo [:mak]]

Attachment: 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.

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.

Comment 1

[Dave Townsend [:mossop]]

(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 :(

Comment 2

[Marco Bonardo [:mak]]

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?)

Comment 3

[Dave Townsend [:mossop]]

(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.

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91c7824254cb

Comment 5

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/91c7824254cb

Comment 6

[Shawn Wilsher :sdwilsh]

(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 :)