1352401 - Expose the number of IDB connection per TabGroup and per window for the decision making of preemption and timer-throttling.

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, enhancement)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

The background:
1. Freezing background tabs with IDB connection opened might cause the the IDB 
   transactions in foreground tabs of the same IDB be blocked if 
   - there is a write transaction with overlapping scope ongoing in background
     tab. [1]
   - the version of IDB connection to be opened in foreground is newer than the 
     one in background tab. (This blocks the schema upgrade of this new open 
     request. [2])

To resolve this problem:
1. we need to expose the number of IDB connections per window for decision making.
2. The connections opened in worker won't be counted because worker will keep 
   running when tab is frozen.

[1] Constraints in http://w3c.github.io/IndexedDB/#transaction-lifetime-concept
[2] Step 10 of http://w3c.github.io/IndexedDB/#opening

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Andreas,

Please leave some hint to me where the central place is to expose this information.

Thanks!

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #1)
> Hi Andreas,
> 
> Please leave some hint to me where the central place is to expose this
> information.
> 
> Thanks!

TabChild seems to be a good option to record the number of IDB connections per tab:
http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/dom/indexedDB/IDBFactory.cpp#171

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] (PTO until 4/10) from comment #2)
> TabChild seems to be a good option to record the number of IDB connections
> per tab:
> http://searchfox.org/mozilla-central/rev/
> 381a7b8f8a78b481336cfa384cb0a5536e217e4a/dom/indexedDB/IDBFactory.cpp#171

Per today's syncup, on Android (non-e10s), we already throttle background tabs, so we'd like to align the behavior with what we'll do on desktop.
Hence, instead of adding this information in TabChild(e10s-only), we should expose this information in docshell similar to
http://searchfox.org/mozilla-central/source/docshell/base/nsIDocShell.idl#642
Besides, this will only be set on the Top-Level docShells in web content and there are 2 possible ways to access them:
- http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/docshell/base/nsDocShell.cpp#1996 
- or use GetSameTypeRootTreeItemIgnoreBrowserBoundaries()

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Keep the number of IDB connections on the Top-Level docshells seems not a good idea because the inner window could be changed when its URL is changed.

A better solution is to have the count of active IDB connections stored per nsGlobalWindow and provide a new method called nsGlobalWindow::GetIDBConnectionCout() which returns the counts of itself plus the accumulation of its sub-windows.
To do this, we might have to refactor nsGlobalWindow::CallOnChildren() to accept a returned value of the accumulated result.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (WIP) Part 1: Turn CallOnChildren to ChildrenIterator for the accumulation of IDB connection.

Due to the concern in comment 4,
I'd like to keep the number of IDB Connections in nsGlobalWindow instead of in the nsDocShell.
So, the IDB connection can still be counted when its owner window is moved to the bfcache.
(The open/close operations of IDB are asynchronous.)

Hi Olli,

May I have your feedback about this change?

Hi Andreas,

I am not sure where the tab freezing logic will be located.

May I know if nsGlobalWindow::CountIDBConnections() or nsPIDOMWindowInner::CountIDBConnections() in this patch is reachable for tab freezing or do we still need to expose these from window to nsIDocShell?

Thanks!

Comment 6

[Andreas Farre [:farre]]

Comment on attachment 8857400 [details] [diff] [review]
(WIP) Part 1: Turn CallOnChildren to ChildrenIterator for the accumulation of IDB connection.

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

Yep, it will/should be reachable from nsGlobalWindow.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

I just realized that there is already an existing mechanism in IDB which was introduced in Bug 618196 to force abort/close the IDB connection if its owner is in bfcache and there is a new connection with newer version opened in another tab:
http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/indexedDB/ActorsChild.cpp#1981-2002

So, for the version change problem mentioned in comment 0 when tab freezing, 
this shall be covered if we can add additional condition at [1] to close the IDB connection.

For the write transaction problem mentioned in comment 0,
if any JS callback of the DOM event will not be triggered when tab is frozen and
the upcoming runnable queued in MetaStableState will be executed [2] without being context-switched by qDOM scheduler,
then this transaction will be committed automatically[3][4] without blocking next transaction.

If these assumption is correct, then all we have to do is to add addition condition at [1] for frozen tab.
(If nsPIDOMWindowInner::IsFrozen() indicates that the tab is frozen, then we don't have to do anything for freezing tab if there is any active IDB connection in a frozen tab.

ni :smaug to clarify whether a JS callback of a DOM event will be called when the tab was frozen.

ni :billm to clarify whether a runnable in event queue and its runnables in MetaStableState will be consumed before switching to another cooperative thread in qDOM scheduler.

[1] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/indexedDB/ActorsChild.cpp#1984
[2] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/indexedDB/IDBTransaction.cpp#194,229
[3] http://searchfox.org/mozilla-central/rev/2fc8c8d483d9ec9fd0ec319c6c53807f7fa8e8a2/dom/indexedDB/IDBTransaction.cpp#1026
[4] http://searchfox.org/mozilla-central/source/dom/indexedDB/IDBTransaction.cpp#390

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Andreas Farre [:farre] from comment #6)
> Comment on attachment 8857400 [details] [diff] [review]
> (WIP) Part 1: Turn CallOnChildren to ChildrenIterator for the accumulation
> of IDB connection.
> 
> Review of attachment 8857400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yep, it will/should be reachable from nsGlobalWindow.

Maybe we don't really need these change after comment 7 is clarified. :)

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Opps, I forget to carry the ni flags while conflict to Andreas comment. :p
NI for comment 7.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #7)
> I just realized that there is already an existing mechanism in IDB which was
> introduced in Bug 618196 to force abort/close the IDB connection
Sorry, its Bug 605019 done 7 years ago instead of Bug 618196.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #7)
> ni :billm to clarify whether a runnable in event queue and its runnables in
> MetaStableState will be consumed before switching to another cooperative
> thread in qDOM scheduler.
If the preemption will be triggered at the middle of a runnable execution instead of the time when the runnable and all its microtasks are done, it seems to break the concept of "Run-to-completion" in [1], isn't it?

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/EventLoop#Run-to-completion

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #11)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #7)
> > ni :billm to clarify whether a runnable in event queue and its runnables in
> > MetaStableState will be consumed before switching to another cooperative
> > thread in qDOM scheduler.
> If the preemption will be triggered at the middle of a runnable execution
> instead of the time when the runnable and all its microtasks are done, it
> seems to break the concept of "Run-to-completion" in [1], isn't it?
> 
If the information to be exposed in this bug will be part of the decision making for preemption in qDOM scheduler as well,
then there won't be a problem to block new transactions in other tabs.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We will be able to switch to another thread when a transaction is pending (before the metastable state queue is processed). So we will need something for that.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857400 [details] [diff] [review]
(WIP) Part 1: Turn CallOnChildren to ChildrenIterator for the accumulation of IDB connection.

Looks reasonable, and one can always access inner window quite easily from docshell or document, wherever we need the information.

And currently we do access the information about audio from inner window
http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/dom/base/TimeoutManager.cpp#38-42


Do we need to worry performance here? I guess iterating child windows should be usually quite fast.

Is there a reason why IDB couldn't always update the top level content window's counter? Like, it could keep a reference to that inner window and just update the counter there.

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Olli Pettay [:smaug] from comment #14)
> Do we need to worry performance here? I guess iterating child windows should
> be usually quite fast.
> 
> Is there a reason why IDB couldn't always update the top level content
> window's counter? Like, it could keep a reference to that inner window and
> just update the counter there.

I put this information per window because the owning inner window is already kept in each IDBFactory,
which makes more sense to me from design perspective that we could get IDB connections per inner window.
Or maybe I could traverse docShell from this inner window to update the counter to the top-level content window when Increase/DecreaseIDBConnection is called.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #13)
> We will be able to switch to another thread when a transaction is pending
> (before the metastable state queue is processed). So we will need something
> for that.

Is this IDB counter on TOP-level inner window good enough for scheduler 
to prevent the preemption on the TabGroups that has active IDB connections?

Or do you expect IDB implementation has to take care something after preemption
is happened and there are active IDB connections suspended?
I can't imagine what IDB can do here to prevent blocking transactions in other tabs
if it's owning thread has been preempted before micro tasks are clear.
This only thing we can do is *not to preempt a thread(TabGroup)* which contains IDB connections, is this correct?

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #16)
> (In reply to Bill McCloskey (:billm) from comment #13)
> > We will be able to switch to another thread when a transaction is pending
> > (before the metastable state queue is processed). So we will need something
> > for that.
> 
> Is this IDB counter on TOP-level inner window good enough for scheduler 
> to prevent the preemption on the TabGroups that has active IDB connections?

Yes, a counter should be enough. Once the transaction is finished, we'll notice that at the next yield point and we can yield then.

> Or do you expect IDB implementation has to take care something after
> preemption
> is happened and there are active IDB connections suspended?
> I can't imagine what IDB can do here to prevent blocking transactions in
> other tabs
> if it's owning thread has been preempted before micro tasks are clear.
> This only thing we can do is *not to preempt a thread(TabGroup)* which
> contains IDB connections, is this correct?

Right.

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 1: Expose a counter of IDB connection in GlobalWindow.

Instead of lazily accumulating IDB connections in WIP patch, this patch provides a simpler version counter API.

It'll be IDB's duty to keep the top-window reference for counting.

Hi Olli,

May I have your review for this change?

Thanks!

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 2: Add Counter API in IDBFactory to update the counter in top window.

To make sure that it will be constant time for scheduler to retrieve the counter from the top-level window, we keep the top-level window in IDBFactory for updating the counter when IDBOpenRequests and IDBDatabases is alive.

Hi Olli,

May I have your review to see if this is the correct way to get the top-level content window in IDBFactory::CreateForWindow()?

Hi Jan,

May I have your review for these change in IDB?

Thanks

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Patch Part 3: Caculate active IDBOpenRequests and IDBDatabases.

Given the fact that qDOM scheduler will preempt a tab group from JS interrupt, to prevent any IDB transactions of the same IDBDatabase in other tabs be blocked we should expose the number of IDBOpenRequests and IDBDatabases per top window for the decision making of the preemption.

To achieve this, I'd like to count according to the life cycle of their actors childs.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

BTW, is there any suggestion for auto-testing?
I am not pretty sure if this can be covered in our automation test.
I've done some manual test locally, and the counter works fine to me.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

As of now I can't think of any way to test this. This needs preemption stuff or so and can then be tested.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8857865 [details] [diff] [review]
(v1) Patch Part 1: Expose a counter of IDB connection in GlobalWindow.

>+  // Increase/Decrease the number of IDB connections for the decision making
>+  // of tab freezing.
reformulate the comment, since we aren't talking about tab freezing.

>+  // Note: this will only be updated on window.top instance.
It might be good to add some assertions/debug-only-code to the methods that no one calls them on non-toplevel windows.

With those, r+

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

We may need to add some helper to do similar to 
http://searchfox.org/mozilla-central/rev/944f87c575e8a0bcefc1ed8efff10b34cf7a5169/dom/audiochannel/AudioChannelService.cpp#1029,1033-1038

Or in fact, we should refactor that so that same method would look for the top level window and check whether audio is playing or whether IDB is active.

Comment 25

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Olli Pettay [:smaug] from comment #24)
> We may need to add some helper to do similar to 
> http://searchfox.org/mozilla-central/rev/
> 944f87c575e8a0bcefc1ed8efff10b34cf7a5169/dom/audiochannel/
> AudioChannelService.cpp#1029,1033-1038
> 
> Or in fact, we should refactor that so that same method would look for the
> top level window and check whether audio is playing or whether IDB is active.

I'd prefer the later one.
Otherwise, this forces the AudioChannelService or IndexedDatabaseManager to be initiated earlier than any IDB or Audio Request is served.

Comment 26

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8857878 [details] [diff] [review]
(v1) Patch Part 3: Caculate active IDBOpenRequests and IDBDatabases.

Cancel the review because there is some window leakage in browser test to be clarified:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e80766b64da879e0cc4ebd1d16d9171e1e71c458

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8857871 [details] [diff] [review]
(v1) Patch Part 2: Add Counter API in IDBFactory to update the counter in top window.

Sorry for cancel the review because there is some window leakage in browser test to be clarified:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e80766b64da879e0cc4ebd1d16d9171e1e71c458

I'd like to clarify whether holding a Top-level window reference in IDFactory or holding an IDBFactory reference in BackgroundDatabaseChild would cause this leakage even though mTopWindow has join the cycle collection in IDBFactory.

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Solution 1: Accumulate active IDBOpenRequests and IDBDatabases on top-level nsGlobalWindow.

For performance concern, we define the counter in nsGlobalWindow but pick the top-level nsGlobalWindow to accumulate the connections.

Comment 29

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Solution 2: Accumulation active IDBOpenRequests and IDBDatabases per nsGlobalWindow.

Comment 30

[Bevis Tseng[:bevis][:btseng](Exited)]

Sorry for deprecating the patches that is review-granted.

After taking more consideration, I've comed out 2 solutions in comment 28 and comment 29 but I'd like to get more suggestion from you to decide which one shall be chosen.

Solution 1 is based on the r+ patches to have the valid counter in the top-level nsGlobalWindow:
- Unnecessary information expose to nsGlobalWindow compared to solution 2
- but with better performance to get the counted result immediately.

Solution 2 reuses nsGlobalWindow::mIndexedDB to have internal counter per window and will be accumulated only when nsGlobalWindow::CountIDBConnections() is called:
- Better design 
- but the performance to traverse of docshell tree for the scheduler has to be taken into consideration.

(ni :billm as well for the opinions from scheduler perspective.)

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Actually, for preemption, we may want to count the IDB state in TabGroup.
That in case timeout management doesn't need the IDB state after all (which seems to be the case).

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Olli Pettay [:smaug] from comment #31)
> Actually, for preemption, we may want to count the IDB state in TabGroup.
> That in case timeout management doesn't need the IDB state after all (which
> seems to be the case).

I just think of one scenario that throttling timer could block further IDBTransaction...
1. IDBDatabase is opened. (IDBDatabases are usually kept alive by web developers for convenient access to the object stores.)
2. versionChange event was received after a new IDBOpenRequest with newer version fired from another new tab.
3. Ignore or set a flag for this versionChange event instead of calling IDBDatabase.close() immediately.
4. Close the IDBDatabase indirectly from a TimerCallback.

I know this is not a good way to use IDB but it's still possible from API perspective.

In this case, I think we should expose this IDB counter API in TabGroup for the scheduler and in nsGlobalWindow for timer-throttling. 

For the counter in TabGroup,
TabGroup can be accessed from IDBFactory::mWindow without Top-level window by leveraging Solution 1.

For the counter in nsGlobalWindow for timer-throttling,
Since TimeoutManager is created per outer window if my understanding is correct,
we should leverage Solution 2 without accumulating its sub-windows in nsGlobalWindow::CountIDBConnections().

Ni :farre as well to double confirm the need of Timer-throttling.

(Sorry for NI so many people at once in these comments but I'd like to ensure that we are all on the same page to expose these counter API properly. :p)

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

For scheduling, I think we want to be able to check this counter really quickly. We'll need to do this check every time we want to switch threads, which might be every millisecond or so. Putting it on the TabGroup would be really nice. I think it would be a mistake to have to iterate over all child windows, since there can be a lot of them.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Ok, so IDB needs to be checked when doing throttling. Then the top level window approach sounds the right one, and whenever the counter changes in such window, it can also change similar counter in TabGroup.

Comment 35

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Olli Pettay [:smaug] from comment #34)
> Ok, so IDB needs to be checked when doing throttling. Then the top level
> window approach sounds the right one, and whenever the counter changes in
> such window, it can also change similar counter in TabGroup.

I agree that we need a counter per TabGroup but I can't see why we need a counter per top-level-inner-window (which counts all IDB connections of itself and its sub-windows) instead of a counter per inner-window if a TimeoutManager throttles timers per inner-window, although nsPIDOMWindowInner::IsPlayingAudio() used by TimeoutManager looks weird to me to check per top-window internally if we look into AudioChannelService::IsWindowActive() called by nsPIDOMWindowInner::IsPlayingAudio():
http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/dom/audiochannel/AudioChannelService.cpp#1033

If we agree that IDB counter shall be counted per window for timer-throttling,
then I'd like to hide this counter in IDBFactory and expose (nsGlobalWindow|nsPIDOMWindowInner)::HasActiveIDBConnections() by checking this internal counter from nsGlobalWindow:mIndexedDB.
For the counter in TabGroup, I think we need a new atomic<uint(32(64)_t> data member for thread-safety if this will be checked by the preemption thread in the scheduler.

Then, every time when IDBFactory::(Increase|Decrease)Connection() is called, these 2 counters will be updated accordingly.

How do you think?

Comment 36

[Bevis Tseng[:bevis][:btseng](Exited)]

Here comes another problem introduced from counting open IDBDatabase connections which reduces the utilized rate of preemption and timer-throttling because it's quite normal for web-developer to hold the IDBDatabase object without calling IDBDatabase.close() until a versionchange event is received.
This implies the fact that any window that opened an IDBDatabase will never be preempted/throttled. :|

For timer-throttling, if 1-sec throttling won't impact too much for closing a IDBDatabase inside a timercallback after versionchange event is received, I'd rather remove IDB from decision making of timer-throttling.

For preemption,
it will be nice if only IDBOpenRequests and IDBTransactions are counted because their lifetime is short.
However, given the fact that preemption can be happened from an JS interrupt, we have to count IDBDatabase instead to prevent the versionchange event being blocked.
Will this low utilized rate of preemption be expected from scheduler's view point?

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

As I said on IRC, throttling is per tab (at least currently). top level pages and iframes can touch each others so it makes sense to have the same throttling model there (at least when dealing with same origin as the top level window.)

Comment 38

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #36)
> For timer-throttling, if 1-sec throttling won't impact too much for closing
> a IDBDatabase inside a timercallback after versionchange event is received,
> I'd rather remove IDB from decision making of timer-throttling.
> 
> For preemption,
> it will be nice if only IDBOpenRequests and IDBTransactions are counted
> because their lifetime is short.
> However, given the fact that preemption can be happened from an JS
> interrupt, we have to count IDBDatabase instead to prevent the versionchange
> event being blocked.
> Will this low utilized rate of preemption be expected from scheduler's view
> point?

After more understanding of how IDB impacts throttling/preemption,
I think we should expose 2 informations:
1. number of active IDBTransactions.
2. number of active IDBDatabase connections.

For timer-throttling, 
only IDBDatabase has to be checked to throttle the timer no more than 1 sec, for example to ensure that IDBDatabase.close() can be invoked from a timercallback.

For preemption,
If there is no IDBTransactions but there is active IDBDatabase, we could preempt TabGroup, for example, in 1~5 secs.
If there is active IDBTransactions, preemption is not allowed.

How do you think?

Comment 39

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> For preemption,
> If there is no IDBTransactions but there is active IDBDatabase, we could preempt TabGroup, for example, in 1~5 secs.
> If there is active IDBTransactions, preemption is not allowed.

Can you describe what would happen if we preempt a tab when it has an active IDBDatabase but no active IDBTransactions? I guess if another tab wants to use the same database with a new version, it would be blocked until the original tab closes its database?

But how does this work normally? It seems like a tab that keeps an IDBDatabase open for a long time (maybe throughout the entire time it's open) could prevent another tab from upgrading the database. Can the upgrade happen even if the original tab has the IDBDatabase open?

Anyway, it seems like maybe we could detect that the new tab is blocked on the original tab finishing and then finish it. That makes scheduling more complicated, but not too much.

Comment 40

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bill McCloskey (:billm) from comment #39)
> > For preemption,
> > If there is no IDBTransactions but there is active IDBDatabase, we could preempt TabGroup, for example, in 1~5 secs.
> > If there is active IDBTransactions, preemption is not allowed.
> 
> Can you describe what would happen if we preempt a tab when it has an active
> IDBDatabase but no active IDBTransactions? I guess if another tab wants to
> use the same database with a new version, it would be blocked until the
> original tab closes its database?
Correct.
> But how does this work normally? It seems like a tab that keeps an
> IDBDatabase open for a long time (maybe throughout the entire time it's
> open) could prevent another tab from upgrading the database. Can the upgrade
> happen even if the original tab has the IDBDatabase open?
IDBDatabase.onversionchange will be triggered on the old IDBDatabase instance if a new IDBOpenRequest with newer verrsion is fired from another script and, at the same time, the new IDBOpenRequest.onblocked will be triggered.
This gives the older script a chance to close this IDBDatabase connection
> Anyway, it seems like maybe we could detect that the new tab is blocked on
> the original tab finishing and then finish it. That makes scheduling more
> complicated, but not too much.
I am not sure how we can detect that the new tab is blocked by a *preempted* background tab but, IMO, if the background tab was preempted, the received VersionChange ipc message sent from parent process will never be handled until resuming the tab.
However, if the number of IDBDatabase is taken into account by the scheduler, we could throttle the cpu time to this background tab instead of preempting it to give it a chance to call IDBDatabase.close after IDBDatabase.onversionchanage is called.

No matter how we are trying to improve, exposing the numbers of IDBDatabase & IDBTransaction individually provides the flexibility for the decision making of preemption and timer-throttling which is what I am going to do in next patch update.

Comment 41

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 1: Define IDB Counters in TabGroup and nsGlobalWindow.

1. Provide IDB Counters per Tab in nsGlobalWindow.
2. Provide IDB Counters per TabGroup in TabGroup class.
3. 2 Counters: one for IDBTransaction; the other for IDBDatabase.

For Timer-throttling,
IDBDatabaseCounter per Tab shall be good enough for the decision makinig.
For Preemption,
Both counters in TabGroup are required to adjust the CPU usage or prevent the preemption as explained in comment 38.

You can peek IDBFactory::(Increase|Decrease)IDB(Transaction|Database) to see how they are counted.

Comment 42

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Part 2: Count active IDBOpenRequest/IDBDatabase/IDBTransaction.

I'd like to count active IDBDatabase and IDBTransaction separately for the decision making explained in comment 38.

May I have your review for these change?

Thanks!

Comment 43

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #41)
> 1. Provide IDB Counters per Tab in nsGlobalWindow.
> 3. 2 Counters: one for IDBTransaction; the other for IDBDatabase.
> 
> For Timer-throttling,
> IDBDatabaseCounter per Tab shall be good enough for the decision makinig.
Maybe we could remove the IDBTransactionCounter if it will never be used by TimeoutManager.

Comment 44

[Bevis Tseng[:bevis][:btseng](Exited)]

Treeherder result for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=427c024de70db52f4011c85c71f2fbc7dc2545e8

Comment 45

[Jan Varga [:janv]]

Comment on attachment 8859521 [details] [diff] [review]
(v2) Part 2: Count active IDBOpenRequest/IDBDatabase/IDBTransaction.

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

Reviewing this today, but give me some time.

Comment 46

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #45)
> Reviewing this today, but give me some time.

This is a blocker to the preemption in scheduler and throttling in timer manager.
Jan, may I know if there is any update about this?

Comment 47

[Jan Varga [:janv]]

Comment on attachment 8859521 [details] [diff] [review]
(v2) Part 2: Count active IDBOpenRequest/IDBDatabase/IDBTransaction.

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

I have a couple of questions/suggestions...

::: dom/indexedDB/IDBFactory.cpp
@@ +442,5 @@
>  
>  #endif // DEBUG
>  
> +void
> +IDBFactory::IncreaseIDBTransaction()

Given the first patch uses IDBTransactionCounter naming schema, should this method (and others) follow that ?
IDBFactory::IncreaseIDBTransactionCount(er) ?

It also seems that the counter is used both for IDBTransaction and also for its actor. Is it really needed to count both ?
If yes, then I will have to double check where we update these counters and the method name(s) might need to be changed.

@@ +459,5 @@
> +  if (mTopWindow) {
> +    MOZ_ASSERT(mTopWindow->IDBTransactionCounter() > 0);
> +    mTopWindow->IDBTransactionCounter()--;
> +    MOZ_ASSERT(mTopWindow->TabGroup()->IDBTransactionCounter() > 0);
> +    mTopWindow->TabGroup()->IDBTransactionCounter()--;

So it's the top level window that knows how to get a TabGroup.
Should the window have a method that would update both counters ?
I think IDB should just use a simple hooks/notifications here, it doesn't have to know that there's also a TabGroup and that both counters need to be updated, don't you think ?

@@ +466,5 @@
> +
> +void
> +IDBFactory::IncreaseIDBDatabase()
> +{
> +  AssertIsOnOwningThread();

Nit: I would add a blank line after all these |AssertIsOnOwningThread();|

Comment 48

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #47)
> Comment on attachment 8859521 [details] [diff] [review]
> (v2) Part 2: Count active IDBOpenRequest/IDBDatabase/IDBTransaction.
> 
> Review of attachment 8859521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have a couple of questions/suggestions...
> 
> ::: dom/indexedDB/IDBFactory.cpp
> @@ +442,5 @@
> >  
> >  #endif // DEBUG
> >  
> > +void
> > +IDBFactory::IncreaseIDBTransaction()
> 
> Given the first patch uses IDBTransactionCounter naming schema, should this
> method (and others) follow that ?
> IDBFactory::IncreaseIDBTransactionCount(er) ?
m... there are multiple counters to increase internally.
IncreaseIDBTransactionCount makes sense here, I can rename it in next update.
> It also seems that the counter is used both for IDBTransaction and also for
> its actor. Is it really needed to count both ?
No, we only count the actors instead in the patch because that's the real connection that would block IDB operations in other windows.
I decrease IDBTransaction in FireCompleteOrAbortEvents() instead of in BackgroundTransactionBase::NoteActorDestroyed() because it's earliest place that the actor becomes non-blocking to other windows even though it can be held for a long time by the script via IDBTransaction. (We want the preemption to be enabled as soon as possible.)
I should add more explanation on this in the comment.

I am also wonder if I should wrap the call of IDBFactory::IncreaseIDBTransaction() in FireCompleteOrAbortEvents() as a runnable and dispatch after mDatabase->DelayedMaybeExpireFileActors() to ensure that we are in a stable state for further preemption in next JS interrupt:
http://searchfox.org/mozilla-central/source/dom/indexedDB/IDBTransaction.cpp#836
> If yes, then I will have to double check where we update these counters and
> the method name(s) might need to be changed.
It will be great help to double check if I increase/decrease IDBTransaction correctly. :-)
> 
> @@ +459,5 @@
> > +  if (mTopWindow) {
> > +    MOZ_ASSERT(mTopWindow->IDBTransactionCounter() > 0);
> > +    mTopWindow->IDBTransactionCounter()--;
> > +    MOZ_ASSERT(mTopWindow->TabGroup()->IDBTransactionCounter() > 0);
> > +    mTopWindow->TabGroup()->IDBTransactionCounter()--;
> 
> So it's the top level window that knows how to get a TabGroup.
> Should the window have a method that would update both counters ?
> I think IDB should just use a simple hooks/notifications here, it doesn't
> have to know that there's also a TabGroup and that both counters need to be
> updated, don't you think ?
I did this by intention to reduce the number of GetXxx/SetXxx in both TabGroup and nsPIDOMWindowInner by having single API to get/set the counter. Can this convince you that it's a good trade-off?
> @@ +466,5 @@
> > +
> > +void
> > +IDBFactory::IncreaseIDBDatabase()
> > +{
> > +  AssertIsOnOwningThread();
> 
> Nit: I would add a blank line after all these |AssertIsOnOwningThread();|
Will do!


P.S. I almost forgot what I have done in this patch. More comment is required in why to decrease the Transaction Actors in FireCompleteOrAbortEvents().

Comment 49

[Jan Varga [:janv]]

ok, please update the patch and I'll look again.

Comment 50

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Part 2: Count active IDBOpenRequest/IDBDatabase/IDBTransaction.

address comment 47 and comment 48.

Comment 51

[Jan Varga [:janv]]

Comment on attachment 8868013 [details] [diff] [review]
(v3) Part 2: Count active IDBOpenRequest/IDBDatabase/IDBTransaction.

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

Some comments

::: dom/indexedDB/IDBFactory.h
@@ +145,5 @@
> +  // making of preemption and throttling.
> +  // Note: *active* here means the state of its actor is not committed or
> +  //       aborted yet and could block IDB operations in other window.
> +  void
> +  IncreaseIDBTransactionCount();

Sorry for being perfectionist, but this comment and the method name is a bit confusing. IDBTransaction is a specific class, so one would think that "number of active IDBTransaction" is number of IDBTransaction objects.
Could we just use "transactions" instead of "IDBTransaction" in the comment ?
Also, the method name could be IncreaseActiveTransactionCount().
The same applies to decreasing.

::: dom/indexedDB/IDBRequest.cpp
@@ +518,5 @@
>    // aOwner may be null.
> +
> +  // Increase active BackgroundFactoryRequestChild.
> +  // Note: we count here instead of the actor's ctor because the preemption
> +  // could happen at next JS interrupt but the 1st PBackgroundIDBFactoryChild is

the 1st PBackgroundIDBFactoryChild ?
Maybe you meant PBackgroundIDBDatabaseChild ?

Comment 52

[Jan Varga [:janv]]

(In reply to Bevis Tseng [:bevis][:btseng] from comment #48)
> > @@ +459,5 @@
> > > +  if (mTopWindow) {
> > > +    MOZ_ASSERT(mTopWindow->IDBTransactionCounter() > 0);
> > > +    mTopWindow->IDBTransactionCounter()--;
> > > +    MOZ_ASSERT(mTopWindow->TabGroup()->IDBTransactionCounter() > 0);
> > > +    mTopWindow->TabGroup()->IDBTransactionCounter()--;
> > 
> > So it's the top level window that knows how to get a TabGroup.
> > Should the window have a method that would update both counters ?
> > I think IDB should just use a simple hooks/notifications here, it doesn't
> > have to know that there's also a TabGroup and that both counters need to be
> > updated, don't you think ?
> I did this by intention to reduce the number of GetXxx/SetXxx in both
> TabGroup and nsPIDOMWindowInner by having single API to get/set the counter.
> Can this convince you that it's a good trade-off?

So you added 4 methods to nsPIDOMWindowInner:
uint32_t& IDBTransactionCounter();
uint32_t& IDBDatabaseCounter();
bool HasActiveIDBTransactions();
bool HasActiveIDBDatabases();

Do we actually need to expose precise number of active transactions/databases ?
I guess we don't need it, especially given that for the database case we count both active database objects and also open database requests.

What about something like this:
void UpdateActiveTransactionCount(uint32_t aDelta)
void UpdateActiveDatabaseCount(uint32_t aDelta)
and that can update both counters, in the window and also in the TabGroup

BTW, the first patch also uses the IDB prefix, but as I mentioned in the previous comment, when I see IDBTransaction I immediately connect that with the IDBTransaction class. Maybe the first patch should also be updated to use something else
maybe IndexedDB instead of IDB.

Comment 53

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8868013 [details] [diff] [review]
(v3) Part 2: Count active IDBOpenRequest/IDBDatabase/IDBTransaction.

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

::: dom/indexedDB/IDBFactory.h
@@ +145,5 @@
> +  // making of preemption and throttling.
> +  // Note: *active* here means the state of its actor is not committed or
> +  //       aborted yet and could block IDB operations in other window.
> +  void
> +  IncreaseIDBTransactionCount();

Will do!

::: dom/indexedDB/IDBRequest.cpp
@@ +518,5 @@
>    // aOwner may be null.
> +
> +  // Increase active BackgroundFactoryRequestChild.
> +  // Note: we count here instead of the actor's ctor because the preemption
> +  // could happen at next JS interrupt but the 1st PBackgroundIDBFactoryChild is

m... it shall be BackgroundFactoryRequestChild instead.
The 1st BackgroundFactoryRequestChild could be created asynchronously from IDBFactory::BackgroundCreateCallback::ActorCreated(), if the PBackgroundChild was not created yet on current thread:
http://searchfox.org/mozilla-central/source/dom/indexedDB/IDBFactory.cpp#946
(PBackgroundChild is lazily created per thread.)

Maybe I should revise the comment as followed:
  // Increase active BackgroundFactoryRequestChild.
  // Note: we count here instead of the actor's ctor because the preemption
  // could happen at next JS interrupt but its BackgroundFactoryRequestChild 
  // could be created asynchronously from IDBFactory::BackgroundCreateCallback
  // ::ActorCreated() if its PBackgroundChild is not created yet on this thread.

Comment 54

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #52)
> So you added 4 methods to nsPIDOMWindowInner:
> uint32_t& IDBTransactionCounter();
> uint32_t& IDBDatabaseCounter();
> bool HasActiveIDBTransactions();
> bool HasActiveIDBDatabases();
> 
> Do we actually need to expose precise number of active
> transactions/databases ?
> I guess we don't need it, especially given that for the database case we
> count both active database objects and also open database requests.
No, only HasActiveIDBTransactions(), HasActiveIDBDatabases() is needed for timer throttling which helps to check the counter in the top-window of current inner window.
The counter will only be accessed by IDBFactory.
> What about something like this:
> void UpdateActiveTransactionCount(uint32_t aDelta)
> void UpdateActiveDatabaseCount(uint32_t aDelta)
If I understand correctly, it seems to be 
  enum CountType {
    Increase,
    Decease
  };
  UpdateActiveTransactionCount(CountType aType);
Then, I can merge {Increase|Decrease}ActiveTransactionCount() into this method.
> and that can update both counters, in the window and also in the TabGroup
> 
> BTW, the first patch also uses the IDB prefix, but as I mentioned in the
> previous comment, when I see IDBTransaction I immediately connect that with
> the IDBTransaction class. Maybe the first patch should also be updated to
> use something else
> maybe IndexedDB instead of IDB.
Agree. I'll update these names.

Comment 55

[Jan Varga [:janv]]

(In reply to Bevis Tseng [:bevis][:btseng] from comment #54)
> If I understand correctly, it seems to be 
>   enum CountType {
>     Increase,
>     Decease
>   };
>   UpdateActiveTransactionCount(CountType aType);
> Then, I can merge {Increase|Decrease}ActiveTransactionCount() into this
> method.

You could also avoid the enum and just use |int32_t aDelta| as the parameter (and pass +1 or -1).
(Sorry, I wrote uint32_t in my previous comment)

Comment 56

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #55)
> (In reply to Bevis Tseng [:bevis][:btseng] from comment #54)
> > If I understand correctly, it seems to be 
> >   enum CountType {
> >     Increase,
> >     Decease
> >   };
> >   UpdateActiveTransactionCount(CountType aType);
> > Then, I can merge {Increase|Decrease}ActiveTransactionCount() into this
> > method.
> 
> You could also avoid the enum and just use |int32_t aDelta| as the parameter
> (and pass +1 or -1).
> (Sorry, I wrote uint32_t in my previous comment)

I was thinking *bool* originally because the aDelta shall always be +/-1 instead of other amount but the readability in bool is bad. Thats why I choose enum. :\

How do you think?

Comment 57

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevis][:btseng] from comment #56)
> (In reply to Jan Varga [:janv] from comment #55)
> > (In reply to Bevis Tseng [:bevis][:btseng] from comment #54)
> > > If I understand correctly, it seems to be 
> > >   enum CountType {
> > >     Increase,
> > >     Decease
> > >   };
> > >   UpdateActiveTransactionCount(CountType aType);
> > > Then, I can merge {Increase|Decrease}ActiveTransactionCount() into this
> > > method.
> > 
> > You could also avoid the enum and just use |int32_t aDelta| as the parameter
> > (and pass +1 or -1).
> > (Sorry, I wrote uint32_t in my previous comment)
> 
> I was thinking *bool* originally because the aDelta shall always be +/-1
> instead of other amount but the readability in bool is bad. Thats why I
> choose enum. :\
> 
> How do you think?

Well, I could always MOZ_ASSERT aDelta and only increase/decrease "1" internally.
I'll do that as you suggested with some comments for |aDelta|.

Comment 58

[Jan Varga [:janv]]

I think we don't have to insist on +1, -1
It can be +N, -N
The assertion could check that we don't go below 0 or over the max int.

Comment 59

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #58)
> I think we don't have to insist on +1, -1
> It can be +N, -N
> The assertion could check that we don't go below 0 or over the max int.
Ok! :)

Comment 60

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v4) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

replace prefix of IDB to IndexedDB.

Comment 61

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v4) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

Replace {Increase|Decrease}IDB{Database|Transaction}Count() with UpdateActive{Database|Transaction}Count(int32_t aDelta);

Comment 62

[Jan Varga [:janv]]

Comment on attachment 8873765 [details] [diff] [review]
(v4) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

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

::: dom/indexedDB/IDBFactory.cpp
@@ +448,5 @@
> +  AssertIsOnOwningThread();
> +  if (mTopWindow && aDelta != 0) {
> +    DebugOnly<uint32_t> count = mTopWindow->IndexedDBTransactionCounter();
> +    mTopWindow->IndexedDBTransactionCounter() += aDelta;
> +    mTopWindow->TabGroup()->IndexedDBTransactionCounter() += aDelta;

I thought that you would now do what I described in comment 47:

"So it's the top level window that knows how to get a TabGroup.
Should the window have a method that would update both counters ?
I think IDB should just use a simple hooks/notifications here, it doesn't have to know that there's also a TabGroup and that both counters need to be updated, don't you think ?"

Comment 63

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v6) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

Address suggestion in comment 62.

Comment 64

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v6) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

Sorry for missing the suggestion mentioned in comment 62. :p

Comment 65

[Jan Varga [:janv]]

Comment on attachment 8875241 [details] [diff] [review]
(v6) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

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

Some comments ...
I still have to double check IDBRequest.cpp and IDBTransaction.cpp

::: dom/indexedDB/IDBDatabase.cpp
@@ +182,5 @@
>    aFactory->AssertIsOnOwningThread();
>    MOZ_ASSERT(aActor);
>    MOZ_ASSERT(aSpec);
> +
> +  // Increase active BackgroundDatabaseChild.

Nit: "Increase the number of active databases"

@@ +183,5 @@
>    MOZ_ASSERT(aActor);
>    MOZ_ASSERT(aSpec);
> +
> +  // Increase active BackgroundDatabaseChild.
> +  mFactory->UpdateActiveDatabaseCount(1);

So we do this in the constructor ...
Now when I look at IDBDatabase::Create() it can fail and return null.
Although, BackgroundDatabaseChild::EnsureDOMObject() assumes that it can fail.
So IDBDatabase increases the number, but since Create() fails, I guess Close() nor ClearBackgroundActor() won't be called and we will end up with an increase/decrease mismatch.

@@ +298,5 @@
>      if (mBackgroundActor && !mInvalidated) {
>        mBackgroundActor->SendClose();
>      }
> +
> +    // Decrease active BackgroundDatabaseChild.

Nit: "Decrease the number of active databases"

@@ +798,5 @@
> +{
> +  AssertIsOnOwningThread();
> +
> +  if (!mClosed) {
> +    // Decrease active BackgroundDatabaseChild if BackgroundDatabaseChild is

Nit: "Decrease the number of active databases ..."

@@ +799,5 @@
> +  AssertIsOnOwningThread();
> +
> +  if (!mClosed) {
> +    // Decrease active BackgroundDatabaseChild if BackgroundDatabaseChild is
> +    // destroyed before ::::CloseInternal() is called.

Nit: Why "::::" ?

Comment 66

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #65)
> Comment on attachment 8875241 [details] [diff] [review]
> (v6) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.
> 
> Review of attachment 8875241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some comments ...
> I still have to double check IDBRequest.cpp and IDBTransaction.cpp
> 
> ::: dom/indexedDB/IDBDatabase.cpp
> @@ +182,5 @@
> >    aFactory->AssertIsOnOwningThread();
> >    MOZ_ASSERT(aActor);
> >    MOZ_ASSERT(aSpec);
> > +
> > +  // Increase active BackgroundDatabaseChild.
> 
> Nit: "Increase the number of active databases"
> 
> @@ +183,5 @@
> >    MOZ_ASSERT(aActor);
> >    MOZ_ASSERT(aSpec);
> > +
> > +  // Increase active BackgroundDatabaseChild.
> > +  mFactory->UpdateActiveDatabaseCount(1);
> 
> So we do this in the constructor ...
> Now when I look at IDBDatabase::Create() it can fail and return null.
> Although, BackgroundDatabaseChild::EnsureDOMObject() assumes that it can
> fail.
I guess you mean it "can't" fail:
http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/dom/indexedDB/ActorsChild.cpp#1776
> So IDBDatabase increases the number, but since Create() fails, I guess
> Close() nor ClearBackgroundActor() won't be called and we will end up with
> an increase/decrease mismatch.
I think we could call factory->UpdateActiveDatabaseCount(1) later at the end of IDBDatabase::Create().
> 
> @@ +298,5 @@
> >      if (mBackgroundActor && !mInvalidated) {
> >        mBackgroundActor->SendClose();
> >      }
> > +
> > +    // Decrease active BackgroundDatabaseChild.
> 
> Nit: "Decrease the number of active databases"
> 
> @@ +798,5 @@
> > +{
> > +  AssertIsOnOwningThread();
> > +
> > +  if (!mClosed) {
> > +    // Decrease active BackgroundDatabaseChild if BackgroundDatabaseChild is
> 
> Nit: "Decrease the number of active databases ..."
> 
> @@ +799,5 @@
> > +  AssertIsOnOwningThread();
> > +
> > +  if (!mClosed) {
> > +    // Decrease active BackgroundDatabaseChild if BackgroundDatabaseChild is
> > +    // destroyed before ::::CloseInternal() is called.
> 
> Nit: Why "::::" ?
Sorry, it's typo. It shall be ::CloseInternal() instead.

Comment 67

[Jan Varga [:janv]]

(In reply to Bevis Tseng [:bevis][:btseng] from comment #66)
> I guess you mean it "can't" fail:

No, IDBDatabase::Create() returns null if it can't add an observer, so it can fail.
But BackgroundDatabaseChild::EnsureDOMObject() incorrectly assumes that it can't fail.

Comment 68

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v7) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

1. Make it clear that IDBDatabase::Create() shall never be failed. (We should already hit MOZ_ASSERT(!mBackgroundActor) in ~IDBDatabase() or  EXCEPTION_ACCESS_VIOLATION_READ to nullptr later if we failed to add an observer for "inner-window-destroyed")
2. Remove unnecessary check to the return value of IDBTransaction::CreateVersionChange() which should never fail.
It could at old revision:
https://hg.mozilla.org/mozilla-central/file/8d40b95ffdbf/dom/indexedDB/IDBTransaction.cpp#l231
But it never does now according to current implementation.
3. Revise the counting of active transactions by utilizing mFiredCompleteOrAbort.

Comment 69

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevis][:btseng] from comment #68)
> 1. Make it clear that IDBDatabase::Create() shall never be failed. (We
> should already hit MOZ_ASSERT(!mBackgroundActor) in ~IDBDatabase() or 
> EXCEPTION_ACCESS_VIOLATION_READ to nullptr later if we failed to add an
> observer for "inner-window-destroyed")
The only possible failure in nsObserverService::AddObserver() to IDBDatabase is NS_ERROR_OUT_OF_MEMORY which also prevents us to go further.

Comment 70

[Jan Varga [:janv]]

Comment on attachment 8875566 [details] [diff] [review]
(v7) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

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

More comments, still have to check IDBTransaction

::: dom/indexedDB/ActorsChild.cpp
@@ +1910,5 @@
>                                          actor,
>                                          request,
>                                          aNextObjectStoreId,
>                                          aNextIndexId);
> +  MOZ_ASSERT(transaction);

I think IDBTransaction::Create() can't fail too, it's not necessary for this bug to work correctly, but if we are doing it for CreateVersionChange() we should also do it for Create().

::: dom/indexedDB/IDBDatabase.cpp
@@ +221,5 @@
>        if (NS_WARN_IF(NS_FAILED(
>              obsSvc->AddObserver(observer, kWindowObserverTopic, false)))) {
>          observer->Revoke();
> +        // This should never happen.
> +        MOZ_CRASH("Failed to add inner-window-destroyed observer!");

Hm, should we just convert |NS_WARN_IF(NS_FAILED| to |MOZ_ALWAYS_SUCCEEDS| then ?

@@ +240,5 @@
>      }
>    }
>  
> +  // Increase the number of active databases
> +  aFactory->UpdateActiveDatabaseCount(1);

I like that you moved this from the ctor to Create().

@@ +799,5 @@
> +IDBDatabase::ClearBackgroundActor()
> +{
> +  AssertIsOnOwningThread();
> +
> +  if (!mClosed) {

Hm, can it happen that CloseInternal() is called after ClearBackgroundActor() ?
I think it can happen. CloseInternal() checks if mBackgroundActor is not null before calling SendClose().
So in theory |mFactory->UpdateActiveDatabaseCount(-1)| would be called twice.

::: dom/indexedDB/IDBRequest.cpp
@@ +520,5 @@
> +  // Increase the number of active IDBOpenRequests.
> +  // Note: we count here instead of the actor's ctor because the preemption
> +  // could happen at next JS interrupt but its BackgroundFactoryRequestChild
> +  // could be created asynchronously from IDBFactory::BackgroundCreateCallback
> +  // ::ActorCreated() if its PBackgroundChild is not created yet on this thread.

Yeah, nice comment.

Nit: IDBOpenDBRequest::CreateForJS() can fail, I know that we only care for the window case in this bug, but it would be just a little bit cleaner to increase the count in CreateForWindow() and CreateForJS(). It would also match how we do it for IDBDatabase

@@ +605,5 @@
>    AssertIsOnOwningThread();
>    MOZ_ASSERT_IF(!NS_IsMainThread(), mWorkerHolder);
>  
> +  // Decrease the number of active IDBOpenRequests.
> +  mFactory->UpdateActiveDatabaseCount(-1);

What if NoteComplete() is not called ?
I think that can happen if InitiateRequest() fails for some reason in IDBFactory.cpp

Comment 71

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #70)
> I think IDBTransaction::Create() can't fail too, it's not necessary for this
> bug to work correctly, but if we are doing it for CreateVersionChange() we
> should also do it for Create().
Yes, thanks for capturing this. I'll replace with MOZ_ASSERT(transaction) after Create() is called at:
http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/dom/indexedDB/IDBDatabase.cpp#699-702
> ::: dom/indexedDB/IDBDatabase.cpp
> @@ +221,5 @@
> >        if (NS_WARN_IF(NS_FAILED(
> >              obsSvc->AddObserver(observer, kWindowObserverTopic, false)))) {
> >          observer->Revoke();
> > +        // This should never happen.
> > +        MOZ_CRASH("Failed to add inner-window-destroyed observer!");
> 
> Hm, should we just convert |NS_WARN_IF(NS_FAILED| to |MOZ_ALWAYS_SUCCEEDS|
> then ?
Will do!
> @@ +799,5 @@
> > +IDBDatabase::ClearBackgroundActor()
> > +{
> > +  AssertIsOnOwningThread();
> > +
> > +  if (!mClosed) {
> 
> Hm, can it happen that CloseInternal() is called after
> ClearBackgroundActor() ?
> I think it can happen. CloseInternal() checks if mBackgroundActor is not
> null before calling SendClose().
> So in theory |mFactory->UpdateActiveDatabaseCount(-1)| would be called twice.
m.. You're right! I should call mFactory->UpdateActiveDatabaseCount(-1) in CloseInternal() only if mBackgroundActor is alive and IDBDatabase itself is not invalidated yet:
http://searchfox.org/mozilla-central/rev/20963d7269b1b14d455f47bc0260d0653015bf84/dom/indexedDB/IDBDatabase.cpp#296
> ::: dom/indexedDB/IDBRequest.cpp
> @@ +520,5 @@
> Nit: IDBOpenDBRequest::CreateForJS() can fail, I know that we only care for
> the window case in this bug, but it would be just a little bit cleaner to
> increase the count in CreateForWindow() and CreateForJS(). It would also
> match how we do it for IDBDatabase
Will do!
> @@ +605,5 @@
> >    AssertIsOnOwningThread();
> >    MOZ_ASSERT_IF(!NS_IsMainThread(), mWorkerHolder);
> >  
> > +  // Decrease the number of active IDBOpenRequests.
> > +  mFactory->UpdateActiveDatabaseCount(-1);
> 
> What if NoteComplete() is not called ?
> I think that can happen if InitiateRequest() fails for some reason in
> IDBFactory.cpp
Thanks for capturing this. This also reminds me to decrease the active number at ::BackgroundActorFailed() as well!
I think we should call UpdateActiveDatabaseCount(-1) at both ::BackgroundActorFailed() and ::InitiateRequest() after IDBRequest::DispatchNonTransactionError() is called.

Comment 72

[Jan Varga [:janv]]

Comment on attachment 8875566 [details] [diff] [review]
(v7) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

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

I think this is it, just explain why we need the runnable.

::: dom/indexedDB/IDBTransaction.cpp
@@ +302,5 @@
> +  } else {
> +    mBackgroundActor.mNormalBackgroundActor = nullptr;
> +  }
> +
> +  if (!mFiredCompleteOrAbort) {

Yeah, the previous patch was missing this check and update.

@@ +867,5 @@
> +  mDatabase->EventTarget()->Dispatch(
> +    NewCancelableRunnableMethod<int32_t>(
> +      "IDBFactory::UpdateActiveTransactionCount", mDatabase->Factory(),
> +      &IDBFactory::UpdateActiveTransactionCount, -1),
> +    NS_DISPATCH_NORMAL);

Hm, I can't figure out why this needs to be a runnable.

Comment 73

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #72)
> I think this is it, just explain why we need the runnable.
> @@ +867,5 @@
> > +  mDatabase->EventTarget()->Dispatch(
> > +    NewCancelableRunnableMethod<int32_t>(
> > +      "IDBFactory::UpdateActiveTransactionCount", mDatabase->Factory(),
> > +      &IDBFactory::UpdateActiveTransactionCount, -1),
> > +    NS_DISPATCH_NORMAL);
> 
> Hm, I can't figure out why this needs to be a runnable.

The IDBDatabase::ExpireFileActors is done with a runnable.
I'd like to make sure that we won't preempt current thread before these file actors are cleaned up.

Comment 74

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v8) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

1. Increase the number of active IDBOpenRequest in ::CreateForWindow() and ::CreateForJS() instead of its ctor.
2. Decrease the number of active IDBOpenRequest if failed in IDBFactory::BackgroundActorFailed() or IDBFactory::InitiateRequest().
3. Fix the problem of calling |mFactory->UpdateActiveDatabaseCount(-1)| twice.
4. Replace |if (NS_WARN_IF(!transaction))| with MOZ_ASSERT(transaction) if it's infallible.
5. Add comment to explain why we need a runnable to decrease the number of active transaction at the end of IDBTransaction::FireCompleteOrAbortEvents().

Comment 75

[Jan Varga [:janv]]

(In reply to Bevis Tseng [:bevis][:btseng] from comment #73)
> (In reply to Jan Varga [:janv] from comment #72)
> > I think this is it, just explain why we need the runnable.
> > @@ +867,5 @@
> > > +  mDatabase->EventTarget()->Dispatch(
> > > +    NewCancelableRunnableMethod<int32_t>(
> > > +      "IDBFactory::UpdateActiveTransactionCount", mDatabase->Factory(),
> > > +      &IDBFactory::UpdateActiveTransactionCount, -1),
> > > +    NS_DISPATCH_NORMAL);
> > 
> > Hm, I can't figure out why this needs to be a runnable.
> 
> The IDBDatabase::ExpireFileActors is done with a runnable.
> I'd like to make sure that we won't preempt current thread before these file
> actors are cleaned up.

Ok, in that case we can do it a bit more efficient. Let me try something.

Comment 76

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8875982 [details] [diff] [review]
(v8) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

Hit assertion on try on Linux/Windows when decreasing the number of active database in IDBDatabase::ClearBackgroundActor():
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d055a7d6414f691b55aff33c459154ce030b3e2a&selectedJob=105732000
which was not caught locally because it was developed on my Macbook. :(

Sorry about that, I'll fix it before asking next review.

Comment 77

[Jan Varga [:janv]]

Attachment: Avoid additional runnable.

I think, something like this could be done to avoid the additional runnable.
Note, I didn't update/move comments.

Comment 78

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #77)
> Avoid additional runnable.
> 
> I think, something like this could be done to avoid the additional runnable.
> Note, I didn't update/move comments.
Thanks for better suggestion to include these 2 tasks included in OnFinishedTransaction().
I'll apply this in next update!

Comment 79

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8875982 [details] [diff] [review]
(v8) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

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

::: dom/indexedDB/IDBDatabase.cpp
@@ +792,5 @@
> +IDBDatabase::ClearBackgroundActor()
> +{
> +  AssertIsOnOwningThread();
> +
> +  if (!mClosed || mInvalidated) {

mClosed and mInvalidated is not good enough to avoid unexpected decreasing because RecvInvalidate() could be fired even SendClose() had been fired which hits the assertion on tryserver.

It seems unavoidable to add an extra flag |mActiveDatabaseCountDecreased| to check before calling mFactory->UpdateActiveDatabaseCount(-1). :\

Comment 80

[Jan Varga [:janv]]

Yeah, I remember I wanted to suggest that, but for some reason I didn't include it in my comments.

Comment 81

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v9) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

1. Increase the number of active IDBOpenRequest in ::CreateForWindow() and ::CreateForJS() instead of its ctor.
2. Decrease the number of active IDBOpenRequest if failed in IDBFactory::BackgroundActorFailed() or IDBFactory::InitiateRequest().
3. Add IDBDatabase::mActiveDatabaseCountDecreased to do |mFactory->UpdateActiveDatabaseCount(-1)| exactly once.
4. Replace |if (NS_WARN_IF(!transaction))| with MOZ_ASSERT(transaction) if it's infallible.
5. Adopt the OnNewTransaction/OnFinishedTransaction approach for more comprehensive finalization of a transaction.

treeherder result looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8d038b2016b4116f4c37622e12b396a86042727

Comment 82

[Jan Varga [:janv]]

I applied the latest patch to my local tree and I'd like to suggest some small changes.
However it will be better if I test it locally and on the try server first and then I'll attach a patch and final review comments.

Comment 83

[Jan Varga [:janv]]

passes locally, now it's on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f811ec28aad58b066696b8732c5acf4aba10d31

Comment 84

[Jan Varga [:janv]]

Sorry, bad push, here's new one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0df9d68661a77cba1426d88812d67eee3d51bf92

Comment 85

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #84)
> Sorry, bad push, here's new one:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0df9d68661a77cba1426d88812d67eee3d51bf92

It's more *indexedDB* flavor similar to {indexedDB|quota}/ActorsParent.cpp. :D

Comment 86

[Jan Varga [:janv]]

Comment on attachment 8876093 [details] [diff] [review]
(v9) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

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

Sorry that this took so much time, but I think we have to be extra careful here.
If I understood the goal correctly, this can really badly affect preemption and timer-throttling if done incorrectly.
Basically, we need more assertions and boolean members to be 100% sure that this stuff works correctly and that one can easily understand the increases/decreases after a while.

::: dom/indexedDB/ActorsChild.cpp
@@ +1910,5 @@
>                                          actor,
>                                          request,
>                                          aNextObjectStoreId,
>                                          aNextIndexId);
> +  MOZ_ASSERT(transaction);

Nit: The blank line doesn't have to be removed.

::: dom/indexedDB/IDBDatabase.cpp
@@ +175,5 @@
>    , mFileHandleDisabled(aRequest->IsFileHandleDisabled())
>    , mClosed(false)
>    , mInvalidated(false)
>    , mQuotaExceeded(false)
> +  , mActiveDatabaseCountDecreased(false)

I think the logic could be reverse: mIncreasedActiveDatabaseCount

@@ +236,5 @@
>      }
>    }
>  
> +  // Increase the number of active databases
> +  aFactory->UpdateActiveDatabaseCount(1);

We can update the boolean member here.

@@ +297,5 @@
>        mBackgroundActor->SendClose();
>      }
> +
> +    // Decrease the number of active databases after closed.
> +    if (!mActiveDatabaseCountDecreased) {

This could be a new method since there's exactly the same block in ClearBackgroundActor().

@@ +993,5 @@
>    }
>  }
>  
>  void
> +IDBDatabase::OnNewTransaction()

Sorry, I changed my mind this should be NoteActiveTransaction()/NoteInactiveTransaction().

@@ +1008,5 @@
>  {
>    AssertIsOnOwningThread();
>  
>    if (!mBackgroundActor || !mFileActors.Count()) {
> +    // We decrease the number of active transactions here instead of

I think this comment doesn't belong here, FireCompleteOrAbortEvents() is more suitable.

@@ +1181,5 @@
> +IDBDatabase::OnFinishedTransactionDelayed()
> +{
> +  ExpireFileActors(/* aExpireAll */ false);
> +
> +  // We decrease the number of active transactions here instead of

Again, I think FireCompleteOrAbortEvents() is more suitable for this comment.

::: dom/indexedDB/IDBFactory.cpp
@@ +868,5 @@
>      info->mRequest->
>        DispatchNonTransactionError(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +    // Decrease the number of active IDBOpenRequests if failed to initiate.
> +    UpdateActiveDatabaseCount(-1);

Hm, could we just do the decrease in NoteComplete()/DispatchNonTransactionError() ?
I'm sure you found all possible places that cause that the actor won't call NoteComplete(), but we should also make sure in the destructor that we decreased the number of active databases.
Yeah, we will have to add another boolean member, but I don't think that will hurt performance much.
Also, it seems that DispatchNonTransactionError() was created exactly for the case when the actor won't be created, so why don't put the decrease into this method ?

::: dom/indexedDB/IDBFactory.h
@@ +149,5 @@
> +  UpdateActiveTransactionCount(int32_t aDelta);
> +
> +  // Increase/Decrease the number of active databases and IDBOpenRequests for
> +  // the decision making of preemption and throttling.
> +  // Note: a non-closed database or a pending IDBOpenRequest could block

Nit: A non-closed ...

::: dom/indexedDB/IDBRequest.cpp
@@ +547,5 @@
> +  // Note: we count here instead of the actor's ctor because the preemption
> +  // could happen at next JS interrupt but its BackgroundFactoryRequestChild
> +  // could be created asynchronously from IDBFactory::BackgroundCreateCallback
> +  // ::ActorCreated() if its PBackgroundChild is not created yet on this thread.
> +  aFactory->UpdateActiveDatabaseCount(1);

I think we can use a boolean member for this too and create a new method for the increase with all the assertions and comments (the same comment will be at one place only).

@@ +612,5 @@
>    AssertIsOnOwningThread();
>    MOZ_ASSERT_IF(!NS_IsMainThread(), mWorkerHolder);
>  
> +  // Decrease the number of active IDBOpenRequests.
> +  mFactory->UpdateActiveDatabaseCount(-1);

We should make sure in the dtor that we decreased the number.

::: dom/indexedDB/IDBTransaction.cpp
@@ +86,5 @@
>    , mMode(aMode)
>    , mCreating(false)
>    , mRegistered(false)
>    , mAbortedByScript(false)
> +  , mFiredCompleteOrAbort(false)

We should check in the dtor that we noted inactive transaction.

@@ +302,5 @@
> +  }
> +
> +  if (!mFiredCompleteOrAbort) {
> +    // Decrease the number of active transactions here if we didn't receive
> +    // Complete message from parent.

So this follows the original code which doesn't expire file actors if we didn't receive the Complete message.
However, I think we should fix that while we are here. We can just call OnFinishedTransaction() here.

::: dom/indexedDB/IDBTransaction.h
@@ +108,5 @@
>  
>    bool mCreating;
>    bool mRegistered;
>    bool mAbortedByScript;
> +  bool mFiredCompleteOrAbort;

I think this deserves its own boolean member.

Comment 87

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #86)
> Comment on attachment 8876093 [details] [diff] [review]
> (v9) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.
> 
> Review of attachment 8876093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry that this took so much time, but I think we have to be extra careful
> here.
> If I understood the goal correctly, this can really badly affect preemption
> and timer-throttling if done incorrectly.
> Basically, we need more assertions and boolean members to be 100% sure that
> this stuff works correctly and that one can easily understand the
> increases/decreases after a while.
Thanks for making these better.
It's really nice to have all these counters decreased with bool member protected(which was what I tried to avoid having more flags introduced earlier but it's really necessary for better maintenance in the future) and wrap them within a dedicated function.

These are all addressed by the patch on try.
After further review, there is only some minor stuff need to be refined:
1. Wrap the increasing of |ActiveDatabaseCount| and its flag with new IDBDatabase::IncreaseActiveDatabaseCount() method even though it's only used in single place to improve the readability.
2. Declare all these {Increase|Decrease}ActiveDatabaseCount() or Note(In)ActiveTransaction() as private if possible.

I'll refine these and do more test before requesting next review.

Comment 88

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v10) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

1. Merge the patch on try. (Thanks again for making it better!)
2. Define IDBDatabase::IncreaseActiveDatabaseCount() for better reading and to be symmetric to ::MaybeDecreaseActiveDatabaseCount().
3. Move newly define methods as private.
4. Keep ClearBackgroundActor() inline after these new counting methods are defined.

Treeherder result looks good, btw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f41c485f54216e27d540f781838c2304e2af4d25&selectedJob=108060121

Comment 89

[Jan Varga [:janv]]

Comment on attachment 8878978 [details] [diff] [review]
(v10) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions.

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

Thanks!

::: dom/indexedDB/IDBRequest.cpp
@@ +545,5 @@
>    return request.forget();
>  }
>  
>  void
> +IDBOpenDBRequest::IncreaseActiveDatabaseCount()

Nit: These are now private, they should be placed after NoteComplete() in this file.

Comment 90

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #89)
> Nit: These are now private, they should be placed after NoteComplete() in
> this file.
Will do when pushing into mozilla-inbound.

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d068cc946d
Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c62253ad1a3
Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions. r=janv

Comment 92

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70d068cc946d
https://hg.mozilla.org/mozilla-central/rev/9c62253ad1a3

Comment 93

[Bevis Tseng[:bevis][:btseng](Exited)]

Unfortunately, this cause a regression in bug 1374675.
ScriptableTop is not reliable for counting. We have to update the count via current inner window(IDBFactory::mWindow) instead.
The Counter in TabGroup can still be accessed IDBFactory::mWindow without any problem.
However, we need to figure a solution to update the count correctly to the TimeoutManager used by IDBFactory::mWindow.

Comment 94

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v7) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

getScriptableTop() is not reliable even while accessing window.indexedDB from 
the script according to the regression in bug 1374675.

Besides, it's 1-1 mapping between nsPIDOMWindowInner and TimeoutManager after 
further review of current implementation:
[1] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/dom/base/nsGlobalWindow.cpp#1019

Compared to what have done in nsPIDOMWindowInner::IsPlayingAudio(), It seems not
necessary for TimeoutManager to disable throttling by checking the counters 
accumulated in top-level nsPIDOMWindowInner but by checking the ones in its 
owning nsPIDOMWindowInner instead.

Hence, for throttling, I'd like to simplify these logic to ensure that the update
to these IDB counters per inner window is reliable and the TimeoutManager could
get these counters reliably as well from without the access to getScriptableTop()
in mWindow.AsInner()->HasActiveIndexedDB{Databases|Transactions}().

Note: For preemption, the IDB counters per TabGroup can still be updated properly
by IDBFactory::mWindow->UpdateActiveIndexed{Database|Transaction}Count() without 
holding a new reference of the ScriptableTop inner window in IDBFactory.
(There shall be no performance concern to the scheduler.)

Comment 95

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v11) Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions. r=janv

Remove IDBFactory::mTopWindow and reuse IDBFactory::mWindow for counting according to the API change in part 1.

Comment 96

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8879852 [details] [diff] [review]
(v7) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

Comment 37 explained why we need the ScriptableTop to count for the throttling.
It seems unavoidable to hold mTopWindow in IDBFactory and count separately. in IDBFactory instead of in nsPIDOMWindowInner.

Jan, if you agree, I'd like to revise these patches again to meet this requirement.

Comment 97

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevis][:btseng] from comment #96)
> Comment on attachment 8879852 [details] [diff] [review]
> (v7) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.
> 
> Comment 37 explained why we need the ScriptableTop to count for the
> throttling.
> It seems unavoidable to hold mTopWindow in IDBFactory and count separately.
> in IDBFactory instead of in nsPIDOMWindowInner.
> 
> Jan, if you agree, I'd like to revise these patches again to meet this
> requirement.

Per offline discussion, we should still hide the counting detail in nsGlobalWindow without exposing ScriptableTop stuff to IDBFactory::mTopWindow.

There are 2 solutions for tab-wise counting that is required by TimeoutManager per comment 37.
1. To hold the top-level inner window inside each inner window:
   - Some cycle collection issue has to be fixed.
    void
    nsPIDOMWindowInner::UpdateActiveIndexedDBDatabaseCount(int32_t aDelta)
    {
      MOZ_ASSERT(NS_IsMainThread());
    
      if (aDelta == 0) {
        return;
      }
    
      // Only active IDB databases could block throttling.
      if (mTopInnerWindow) {
        mTopInnerWindow->mNumOfIndexedDBDatabases += aDelta;
      }
    
      TabGroup()->IndexedDBDatabaseCounter() += aDelta;
    }

2. To call getScriptableTop() everytime when nsIPDOMWindowInner::UpdateActiveIndexedDBDatabaseCount() is called.
   - The risk is that we don't know if getScriptableTop() is always available in each UpdateActiveIndexedDBDatabaseCount() call
     which causes the counter to be updated incorrectly.
    void
    nsPIDOMWindowInner::UpdateActiveIndexedDBDatabaseCount(int32_t aDelta)
    {
      MOZ_ASSERT(NS_IsMainThread());
    
      if (aDelta == 0) {
        return;
      }
    
      nsGlobalWindow* window = nsGlobalWindow::Cast(AsInner());
  
      MOZ_ASSERT(window);
  
      // Only active IDB databases could block throttling.
      if (nsCOMPtr<nsPIDOMWindowOuter> topOutter = window->GetScriptableTop()) {
        if (nsCOMPtr<nsPIDOMWindowInner> mTopInnerWindow = topOutter->GetCurrentInnerWindow()) {
          mTopInnerWindow->mNumOfIndexedDBDatabases += aDelta;
        }
      }
    
      TabGroup()->IndexedDBDatabaseCounter() += aDelta;
    }

I'll do more experiments to figure out a better solution.

Comment 98

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v8) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

Due to the concern of exposing ScriptableTop to IDBFactory and the regression we hit in bug 1374675, in this patch, we are going to:
1. Hide the IDB counting detail inside nsPIDOMWindowInner/nsGlobalWindow instead of Exposing ScriptableTop to IDBFactory.
2. Note mTopInnerWindow when a new inner window is bound to a outter window.
3. Do the IDB counting on mTopInnerWindow if available. Otherwise, the counting will be done on owning inner window.
4. Count active databases for throttling. (Only active databases could block throttling.)
5. Count databases and transactions for scheduling.

Treeherder result looks good, btw:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c892cb8ad9a012f9ab1ce26ba1d78fcf7244b8

Comment 99

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8880222 [details] [diff] [review]
(v8) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

>+nsPIDOMWindowInner::NoteTopInnerWindow()
>+{
>+  nsGlobalWindow* window = nsGlobalWindow::Cast(AsInner());
>+
>+  MOZ_ASSERT(window);
>+
>+  if (nsCOMPtr<nsPIDOMWindowOuter> topOutter = window->GetScriptableTop()) {
>+    mTopInnerWindow = topOutter->GetCurrentInnerWindow();
>+  }
>+}

Why we always assign mTopInnerWindow?

Also, the name of the method is odd. "Note", we don't note the top inner window.
Could it be CacheTopInnerWindow?


What happens if the window is accessed after it has been removed from its parent?
I think the method should return early in that case.

Comment 100

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v9) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

1. Make sure that mTopInnerWindow can only be set once.
2. Rename the method name to TryToCacheTopInnerWindow() because GetScriptableTop() could return nullptr.
3. Add MOZ_ASSERT(!mInnerObjectsFreed) to ensure that mTopInnerWindow won't be set after InnerWindow is freed.

Treeherder result looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=910e93cb88431c01f854dbc5f0e94889159024a6

Comment 101

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8880691 [details] [diff] [review]
(v9) Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow.

"could still be null and we don't want it to be set multiple times"
I don't quite understand why word "still" there. Perhaps drop that.

Comment 102

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13aa3c43fdc9
Part 1: Define IndexedDB Counters in TabGroup and nsGlobalWindow. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e435906b46
Part 2: Count active IDBOpenRequest, IndexedDB databases/transactions. r=janv

Comment 103

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13aa3c43fdc9
https://hg.mozilla.org/mozilla-central/rev/a4e435906b46