Allow Pages with IndexedDB Connections/Transactions into BFCache

[Mingyu Lei]

Hi IndexedDB owners,

As discussed by yuzus@, dmurph@, jbell@ and fergal@ from the GitHub issue [1], it is feasible to allow pages with IndexedDB connection or outgoing transaction into BFCache, as long as we evict them at the right time.

I have created a doc [2] explaining our current proposal, and we need some help from the storage team to review and provide suggestions, especially to the changes that we propose to make on the IndexedDB components.

Thanks a lot in advance.

Best regards,

Mingyu

[1]: How should IDB connections and outstanding IDB transactions behave when a page enters back/forward cache?

[2]: https://docs.google.com/document/d/182KlafoJMGeYe48GkOTMuJ76WhXMNmOvlp79eWEYkiA/edit?usp=sharing

[Mingyu Lei]

+Daniel Cheng 

Hi Daniel,

I'm doing some changes to unblock BFCache for IndexedDB connection, but encountered some issue with the task queue and mojo call, last time Rakina told me that I can find you for mojo related stuff so I'm sending this email :)

The situation is that we may want to evict a page from renderer side when it's in BFCache and receives a version change event through mojo (which will block the sender), but currently the receiver is running in a pausable queue so the flow is not complete. I tried to set the freezable property to false and it's working as expected [1]. I'm wondering if there is any guideline on setting this as it may bring some privacy concerns.

For more information about the background you can refer to the doc mentioned in the earlier thread (and I also tagged you from the doc)

[1]: https://chromium-review.googlesource.com/c/chromium/src/+/3895322/2/third_party/blink/renderer/platform/scheduler/main_thread/frame_scheduler_impl.cc

Thanks,

Mingyu

[Kentaro Hara]

Thanks Mingyu for looking!

It's not really ideal to make the task queue unfeezeable because we are freezing the task queue to ensure that no tasks run in the BFcached page. Instead of doing this in the renderer side, would it be possible to make the browser process evict the BFcached page before it sends the version change event?

--
You received this message because you are subscribed to the Google Groups "bfcache-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bfcache-dev...@chromium.org.
To view this discussion on the web, visit https://groups.google.com/a/chromium.org/d/msgid/bfcache-dev/CAN_fHt%3DuSWaNgdB5-Bhs1wky3GbyaSc3p00ozJK6nwKmmbcVRA%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[Mingyu Lei]

Yes it should be feasible as well if our proposal of refactoring IDB service from the browser process sounds good to the storage team.

I have added this as the second alternative in the proposal section of the doc before. Thanks!

[Evan Stade]

Sorry for the delayed response --- besides being distracted at TPAC last week, this required a lot of poking around as I'm still relatively new to the IDB codebase. Thanks for the careful and considered write-up. I left a comment on the doc about how to possibly access the RFH id, although I couldn't find an elegant suggestion. Basically it boils down to taking advantage of receiver_set_, which allows you to associate arbitrary data with the source of an incoming mojo call, plus a whole lot of new plumbing. It does seem the case that this sort of direct access to a RFH would break the potential servicifation of IDB. Thus I do like Kentaro's suggestion of an adapter component, perhaps wrapping mojo::PendingAssociatedRemote<blink::mojom::IDBDatabaseCallbacks> database_callback_remotes ?

-- Evan Stade

[Mingyu Lei]

Hi Evan,

Thanks for providing the thoughts and suggestions, and I agree that it's better to have the adapter component. I think for now we have split the project into a few milestones/topics of discussion, namely:

1. How to store RFH id in the connection
2. What could be the best eviction strategy for the page with IDB connection/transaction from BFCache
3. How to evict the page from IDB service while preserving the good isolation for potential servicification in the future

I had highlighted some sections in the design doc which corresponds to the questions above, let's follow up the discussion in the doc thread.

Best regards,

Mingyu

[Mingyu Lei]

Hi Nathan and the Storage team,

I notice that recently there are some major refactoring happening on the lock manager side, so I think maybe I should bump this discussion up again, especially regarding how we manager some reverse relationship between the lock entry and the lock holder (see the comment here: https://docs.google.com/document/d/182KlafoJMGeYe48GkOTMuJ76WhXMNmOvlp79eWEYkiA/edit?disco=AAAAf26hSfc )

This may not be directly related to the lock manager changes but any suggestions would be greatly appreciated.

Thanks,

Mingyu

[Daniel Murphy]

I think it's reasonable to request the lock holders (or some sort of identifying information) given a lock id.

I also share your concern on code executing on destruction. I would hope that the locks would release in a way that schedules the next one asynchronously?

[Mingyu Lei]

Thanks Daniel for the reply. 

Actually I have been working on the proposal with more details and have a separate doc about them. In that I think we don't have to care too much about the destruction since the lock holder will then be maintained in the lock struct itself. I shared that in a separate email before but probable it didn't reach everyone in the thread so maybe I should paste it here again: https://docs.google.com/document/d/1gelstQEmImk9MDI8VpdcHSN9DsrSesvfaId36n0aI0E/edit?usp=sharing

In summary, we need to modify the indexed db code and lock manager as follows:

1. Implement a function in lock manager to query the pending queue size given lock request

2. Observe the BFCache state change from renderer's IDB module and notify IDB service to do eviction check

3. Add holder set into the lock struct and maintain it in lock acquisition and release

4. Accept a blocking callback in the IDB transaction scheduling method as well as the lock acquisition method

There is more information about the background and the rationale about those changes in the doc, pseudocode is also added for clarity. Please help to take a look at your convenience.

Also thanks @Evan Stade for all the help in the previous IDB CLs, if you have time, please help to review this doc as well, as it's based on the previous work.

Best regards,

Mingyu

[Mingyu Lei]

Hi Daniel and the storage team,

 

I think it's reasonable to request the lock holders (or some sort of identifying information) given a lock id.

Originally I was thinking to maintain a set of lock holders, and tried to have a experimental CL (https://crrev.com/c/4022986), however I just found that wouldn't work if we store the weak pointer of lock holder struct, since the weak pointer itself is not hashable.

I'm wondering if it's possible to restrict the lock holder creation from the lock manager only, and each of the lock holder struct will come with a unique id? Or do you have any other better idea on how to achieve "requesting the lock holders given a lock id"? Thanks.

Best regards,

Mingyu