Closed Bug 1604528 Opened 8 months ago Closed 4 months ago

IndexedDB inspector no longer shows main process data

Categories

(DevTools :: Storage Inspector, defect, P2)

defect

Tracking

(firefox-esr68 unaffected, firefox74 wontfix, firefox75 wontfix, firefox76 fixed)

RESOLVED FIXED
Firefox 76
Tracking Status
firefox-esr68 --- unaffected
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- fixed

People

(Reporter: andreio, Assigned: jdescottes)

References

(Blocks 2 open bugs)

Details

(Keywords: regression)

Attachments

(1 file)

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

Trying to inspect stored IndexedDB data.

Please tell us what site you were on, and what steps led to the error you are reporting

No website in particular, inspecting the IndexedDB main process data stored for the newtab page

  1. Open the Browser Toolbox
  2. Navigate to Storage -> IndexedDB
  3. There should be a ActivityStream entry

What happened?

No data is shown (but it is there).

What should have happened?

Usually main process IndexedDB data was available at all times in the Browser Toolbox.
To see this

Cu.import("resource://activity-stream/lib/ASRouter.jsm")
ASRouter._storage.getAll().then(console.log)

Anything else we should know?

Regression but I unfortunatelly don't have a range for this.

@andreio You can set dom.indexedDB.storageOption.enabled to true, open about:newtab and then inspect the indexedDB through the storage panel (you don't need to use the browser toolbox).

We were asked to remove Remove DevTools support for IndexedDB "persistent" storage in bug 1594810 by Simon Giesecke, who is on the storage team.

@Simon can explain why it was necessary to remove DevTools support for persistent storage? A number of Firefox engineers are missing this feature.

Flags: needinfo?(sgiesecke)

The data is stored in the main process not the content process. There is a RemotePages communication channel where the main process sends down data to any of the content processes opened with about:newtab.

Flags: needinfo?(mratcliffe)

Maybe there was a misunderstanding. We are planning to remove the custom overload which explicitly opens an IndexedDB database database using IDBFactory.open(name, { storage: permanent}, and I was asked by :johannh to remove this support. There remain uses of IndexedDB which are implicitly permanent, and these should probably still be accessible via DevTools. Maybe these require the use of the custom open overload aa of now?

Flags: needinfo?(sgiesecke) → needinfo?(jhofmann)

@sg As far as I know there is no way to tell whether a DB is opened using IDBFactory.open(name, { storage: permanent} or opened implicitly using IDBFactory.open(name).

I could add support again but it will still display both "types" of permanent DBs. At least then Firefox devs would still have an easy way to look at our internal DBs.

What do you think?

Flags: needinfo?(mratcliffe) → needinfo?(sgiesecke)

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #4)

@sg As far as I know there is no way to tell whether a DB is opened using IDBFactory.open(name, { storage: permanent} or opened implicitly using IDBFactory.open(name).

Yes, that's true.

I could add support again but it will still display both "types" of permanent DBs. At least then Firefox devs would still have an easy way to look at our internal DBs.

What do you think?

I am curious how the browser toolbox identifies which IndexedDB databases exist? At the moment, the IDBFactory.databases method is not implemented by us.

I am not sure what you mean by both "types" of permanent DBs. These are not different types of DBs, but only different syntactical ways of opening them, which are equivalent in the end. It would certainly be good to be able to browse the system IndexedDB databases again, which are implicitly permanent.

Flags: needinfo?(sgiesecke)

It's been some time since I looked at the storage code but IIRC it's fair to say that using the old way (pre quota-permission) there were two "types" of databases (persistent and temporary) as they were distinguished by that feature in their database name and couldn't easily switch from being one or the other. Maybe I'm misremembering, though.

I agree we should continue to support showing old-style permanent DBs for chrome consumers, but I would expect that to happen at the DOM API level, no?

Flags: needinfo?(jhofmann)

The priority flag is not set for this bug.
:miker, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(mratcliffe)

So, let's bring it back.

Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Flags: needinfo?(mratcliffe)
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All

Ok looking at your patch now there seems to have been a bigger misunderstanding. This is definitely not specifically about the "persistent" part, it's about specifying databases as either temporary or persistent in these APIs. We are removing the permission that will give access to persistent databases for anything but chrome contexts (where it will be given automatically). Additionally, I think Simon wants to remove the storage parameter itself as well. So the change we actually need is:

  • Remove all traces of "temporary" storage
  • Remove the "storage" parameter from your methods that open or deal with databases

The above commit should definitely be backed out, I agree.

Ah, that makes sense.

We will land this to revert the old patch and address anything that mentions temporary storage inside storage.js in Bug 1608825.

Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/83736c0c967d
IndexedDB inspector no longer shows main process data r=jdescottes,johannh

The backout is because of a race condition in completely unrelated code but my patch seems to align the planets in such a way that the race condition occurs consistently on Linux ASAN builds.

The code is completely unrelated but I can sometimes trigger the error using ./mach mochitest devtools/client/accessibility/test/browser/browser_accessibility_panel_highlighter_multi_tab.js --verify.

I'll patch the race condition in order to land the patch.

Flags: needinfo?(mratcliffe)

When the Accessibility panel is disabled via the settings panel the panel to the left of it loads. My changes to the storage panel probably change loading time by a couple of milliseconds but that seems to be enough to trigger this race condition.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:miker, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(michael)

The patch bounced because it broke devtools/client/accessibility/test/browser/browser_accessibility_panel_highlighter_multi_tab.js on Linux ASAN builds.

I have no time to work on this right now... unassigning.

Assignee: michael → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(michael)

Hey, any chance someone else could pick this up?

Flags: needinfo?(jdescottes)

Thanks for the ping, I will try to move this forward.

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Flags: needinfo?(jdescottes)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91df3576d1e3
IndexedDB inspector no longer shows main process data r=jdescottes,johannh
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 76

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
You need to log in before you can comment on or make changes to this bug.