Closed Bug 1471375 Opened 6 years ago Closed 6 years ago

Reports about missing activity stream content on new tab page and about:preferences#home panel

Categories

(Firefox :: New Tab Page, defect, P1)

61 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 63
Iteration:
63.1 - July 9
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 + verified
firefox62 --- verified
firefox63 --- verified

People

(Reporter: philipp, Assigned: andreio)

References

Details

(Keywords: regression)

User Story

nightly: https://github.com/mozilla/activity-stream/commit/ce7f00d085f9318335a84efb7b9b8cb8326f1fdc

beta: https://github.com/mozilla/activity-stream/compare/firefox-62b3...firefox-62b5

release: https://github.com/mozilla/activity-stream/compare/firefox-61b14...firefox-61.0.1

Attachments

(9 files)

in the first hours of the 61.0 rollout we got two independent reports from users that their new tab page was missing the content from activity stream, and the home section within the preferences panel was lacking configuration options regarding activity stream (see attached screenshots).

https://support.mozilla.org/en-US/questions/1223289
https://old.reddit.com/r/firefox/comments/8tzu8l/firefox_610_got_released_release_notes/e1c0o07/

so far in the sumo thread we've established that the error seems to be profile-specific but also occurring during firefox safe mode.
the web console stays blank while loading the new tab page, but the browser console has a number of entries that might be related: https://pastebin.mozilla.org/9088524
Also worth seeing that some user report shows the preloaded content.
My thoughts were that it could be related to IndexedDB but I expected [0] to prevent this for happening. Also I expect any errors thrown in a component to result in the error boundary [1] showing up. No other ideas at the moment.

[0] https://github.com/mozilla/activity-stream/commit/922de73f135a30e68f3aa2938ad13032a42a31c5
[1] https://github.com/mozilla/activity-stream/blob/65898e0cbdc2def10538e48ffef6065d22094f01/content-src/components/Base/Base.jsx#L6
Kate, any idea what might be going on here?
Flags: needinfo?(khudson)
Andrei, can you find someone on your team to try and reproduce this issue? Thanks!
Flags: needinfo?(andrei.vaida)
Based on the error reports, the preferences not showing up in about:preferences#home would suggest something main process related, but I'm not sure what. I'll take a look at our indexedDB crash reports to see if there is anything unusal there
Flags: needinfo?(khudson)
the affected sumo user from the thread in comment #0 has run the mozregression tool and that came up with bug 1455216 as having introduced the regression.

i will also tag other sumo threads i'm suspecting to be about the same issue, so that they show up under https://support.mozilla.org/en-US/questions/firefox?owner=all&tagged=bug1471375&show=all
Blocks: 1455216
The lib/ changes from that export bug 1455216:

* 8657d418 - feat(highlights): Add include* prefs to let about:preferences control what gets shown (#4111) (10 weeks ago) <Ed Lee>
* 50fa46b4 - Fix Bug 1449223 - Add Telemetry for failed IndexedDB transactions (10 weeks ago) <Andrei Oprea>
* e87a190e - feat(highlights): Lazily add DownloadsManager view to handle recent downloads (#4076) (10 weeks ago) <Ursula Sarracini>
* 86050a1c - fix(prefs): Switch to new string for "Sponsored Stories" (#4108) (10 weeks ago) <Ed Lee>
* 60107be6 - fix(stories): Enable for US region only (for en-* locales) (#4105) (2 months ago) <Ed Lee>
* 9e82237f - chore(highlights): Update fetchHighlights comment to explain more (#4104) (2 months ago) <Ed Lee>
* 1a3f8d66 - Fix Bug 1450875 - Wait for Sections to be added before broadcasting content (2 months ago) <Andrei Oprea>
https://github.com/mozilla/activity-stream/commit/8657d4185a031f3bfe925ad767e6653b221b320b
* 8657d418 - feat(highlights): Add include* prefs to let about:preferences control what gets shown (#4111) (10 weeks ago) <Ed Lee>

https://github.com/mozilla/activity-stream/commit/50fa46b44608ac5e19a5d1d4af83b4ee29eb724a
* 50fa46b4 - Fix Bug 1449223 - Add Telemetry for failed IndexedDB transactions (10 weeks ago) <Andrei Oprea>

https://github.com/mozilla/activity-stream/commit/e87a190e15becece8b98a3f49ccf55ffbe9baa1a
* e87a190e - feat(highlights): Lazily add DownloadsManager view to handle recent downloads (#4076) (10 weeks ago) <Ursula Sarracini>

https://github.com/mozilla/activity-stream/commit/86050a1c4d471d269a4cc6efd50400a6dc7f8b51
* 86050a1c - fix(prefs): Switch to new string for "Sponsored Stories" (#4108) (10 weeks ago) <Ed Lee>

https://github.com/mozilla/activity-stream/commit/60107be6446aed067a7855684c9e878aec9ef25c
* 60107be6 - fix(stories): Enable for US region only (for en-* locales) (#4105) (2 months ago) <Ed Lee>

https://github.com/mozilla/activity-stream/commit/9e82237f234f67a2386f15e76a30477c3af90232
* 9e82237f - chore(highlights): Update fetchHighlights comment to explain more (#4104) (2 months ago) <Ed Lee>

https://github.com/mozilla/activity-stream/commit/1a3f8d6651a6c653a7272047594e8afd65c49cb6
* 1a3f8d66 - Fix Bug 1450875 - Wait for Sections to be added before broadcasting content (2 months ago) <Andrei Oprea>

And here's the whole range just in case it isn't one of those… 
https://github.com/mozilla/activity-stream/compare/5f5872876ebd3fcbd7816e079cb34fccb2038620...firefox-61b1
philipp, is there a standard / safe way for users to try getting rid of indexeddb storage? If so, could you ask the sumo reporter to try that?

I believe this would work:

1) about:support
2) click on the profile directory button
3) quit firefox
4) open the profile directory
5) rename "storage" directory to "storage.bak"
6) open firefox
Flags: needinfo?(madperson)
See Also: → 1458320
For diagnosing problems, I'd suggest pointing users at https://firefox-storage-test.glitch.me/ (note that https is essential).

It will indicate if there's a QuotaManager-wide problem or localized problems.  If there are quota-manager problems, then we expect a number of messages from QuotaManager to show up in the browser console.  See "error reporting" near the bottom of https://public.etherpad-mozilla.org/p/quota-manager-schema-change-log for more information on what the output might look like.  (Ignore the wiki page comments, I need to re-run an import so the wiki page is more readable.)

Note that it appears ActivityStream should already be recovering from any problems that are not QuotaManager-wide thanks to the call to deleteDatabase at https://github.com/mozilla/activity-stream/commit/922de73f135a30e68f3aa2938ad13032a42a31c5#diff-cd44612ed8f593a34a8c7021f3fbffbeR83.  Although, note that if that's not a shim that's being referenced but the indexedDB factory global, "await IndexedDB.deleteDatabase(...)" is somewhat wrong because deleteDatabase returns a request, not something that's promise-y.  That said, things should still work out because the open call should just get in the queue behind the deletion completing.

Moving "storage" to "storage.bak" should resolve QuotaManager or IndexedDB related breakage but you'll want to make sure the users delete "storage.bak" afterwards if they're not going to try and correct whatever is going wrong in the directory because the directory will be full of privacy-sensitive information that will no longer be able to be cleared from within the Firefox UI.
Uh, and the browser console messages will only show up at startup and are likely to be evicted by the other browser console spam that happens during the course of browsing.  So please ask users to restart the browser before checking the browser console.
Are there particular Telemetry probes we could be looking at to indicate how common this issue is for users? Trying to decide how high the priority is for when we un-throttle updates.
AS telemetry reports the indexdb error (TRANSACTION_FAILED) in the 61 release. Note that this error did not occur in the 60 release. 

+------------+--------------------+---------+-----------------+
| date       | event              | count   | user_affected   |
|------------+--------------------+---------+-----------------|
| 2018-06-28 | TRANSACTION_FAILED | 71917   | 6659            |
| 2018-06-27 | TRANSACTION_FAILED | 51069   | 5269            |
| 2018-06-26 | TRANSACTION_FAILED | 11978   | 1420            |
| 2018-06-25 | TRANSACTION_FAILED | 7058    | 687             |
| 2018-06-22 | TRANSACTION_FAILED | 6013    | 569             |
| 2018-06-23 | TRANSACTION_FAILED | 4750    | 495             |
| 2018-06-24 | TRANSACTION_FAILED | 4461    | 460             |
+------------+--------------------+---------+-----------------+
I did some more investigation from the QuotaManager/IndexedDB side of the house as it relates to the telemetry, and it looks like this might be some type of bug in the ActivityStream logic rather than QuotaManager or IndexedDB encountering failures.

Specifically:
- The telemetry events seem to be distributed across our platforms in proportion to usage, which helps rule out issues like QuotaManager breaking due to platform-specific files living under PROFILE/storage/ or weird anti-virus stuff.
- All of the telemetry events are for "TRANSACTION_FAILED".  This is one of the two types of event types that can be logged.  The other is "INDEXEDDB_OPEN_FAILED", which is logged from https://hg.mozilla.org/releases/mozilla-release/file/tip/browser/extensions/activity-stream/lib/ActivityStreamStorage.jsm#l81.
- Per guidance from :nanj, I enabled telemetry logging to the browser console by flipping the "browser.ping-centre.log" pref to true for the following experiments.
- If I generate a synthetic QuotaManager failure by altering the QuotaManager's PROFILE/storage.sqlite SQLite schema version (via `PRAGMA user_version = NNN;`), I am able to reproduce an ActivityStream GUI failure, but it results in an "INDEXEDDB_OPEN_FAILED" telemetry event.
- If I generate a synthetic IndexedDB open failure by altering the schema version of PROFILE/storage/permanent/chrome/idb/1657114595AmcateirvtiSty.sqlite, I get an "INDEXEDDB_OPEN_FAILED" telemetry event, but the recovery logic that deletes the database and then attempts to re-create it successfully does its thing and ActivityStream ends up working.

This suggests two major possibilities:
a) A write failure is happening due to an attempt to store something in IndexedDB that can't actually be structured-cloned.  Like an object with a function as a property.  Presumably there is some emergent behavior that would cause this.
b) A read failure is happening at runtime (not open time) due to an attempt to pull something out of IndexedDB that is now an illegal structured clone.  Bug 1458320 happened during Firefox 61 to change JS structured clone disk representations, but really should not be a problem unless we're looking at other structured-clone changes.
In terms of mitigation, for 'a', well, honestly, that probably shouldn't cause catastrophic failure unless there are writes very early in the startup process that could have bad things annotated onto them.

For 'b', the answer would be to delete the IndexedDB database whenever anything bad happens and then re-open the database or refresh the page.  So the the code at https://hg.mozilla.org/releases/mozilla-release/file/tip/browser/extensions/activity-stream/lib/ActivityStreamStorage.jsm#l92 would want to trigger database deletion.  It would also be pretty nice to get a somewhat more informative error from the request.
If it is IndexedDB related, the only commit that touches ActivityStreamStorage.jsm is

* 50fa46b4 - Fix Bug 1449223 - Add Telemetry for failed IndexedDB transactions (10 weeks ago) <Andrei Oprea>
https://github.com/mozilla/activity-stream/commit/50fa46b44608ac5e19a5d1d4af83b4ee29eb724a

That added the event that we're seeing for `async _requestWrapper(request) {`. Although interestingly, if I just throw the stack from just inside requestWrapper, there are some uncaught exceptions for PrefsFeed and SnippetsFeed, but the page and preferences load just fine:

```
Problem getting stored prefs for feeds.section.topstories SectionsManager.jsm:153

Problem getting stored prefs for feeds.section.highlights SectionsManager.jsm:153

uncaught exception: _requestWrapper@resource://activity-stream/lib/ActivityStreamStorage.jsm:89:11
_getAll@resource://activity-stream/lib/ActivityStreamStorage.jsm:49:12
_setPrerenderPref@resource://activity-stream/lib/PrefsFeed.jsm:39:34
init@resource://activity-stream/lib/PrefsFeed.jsm:110:5
onAction@resource://activity-stream/lib/PrefsFeed.jsm:131:9
_middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
init@resource://activity-stream/lib/Store.jsm:140:7

uncaught exception: _requestWrapper@resource://activity-stream/lib/ActivityStreamStorage.jsm:89:11
_getAll@resource://activity-stream/lib/ActivityStreamStorage.jsm:49:12
_setPrerenderPref@resource://activity-stream/lib/PrefsFeed.jsm:39:34
onAction@resource://activity-stream/lib/PrefsFeed.jsm:141:9
_middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
updateTheme@resource://activity-stream/lib/ThemeFeed.jsm:39:5
init@resource://activity-stream/lib/ThemeFeed.jsm:16:5
onAction@resource://activity-stream/lib/ThemeFeed.jsm:45:9
_middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
init@resource://activity-stream/lib/Store.jsm:140:7

uncaught exception: _requestWrapper@resource://activity-stream/lib/ActivityStreamStorage.jsm:89:11
_get@resource://activity-stream/lib/ActivityStreamStorage.jsm:45:12
init@resource://activity-stream/lib/SnippetsFeed.jsm:172:38
onAction@resource://activity-stream/lib/SnippetsFeed.jsm:199:9
_middleware/</<@resource://activity-stream/lib/Store.jsm:51:11
Store/this[method]@resource://activity-stream/lib/Store.jsm:29:55
init@resource://activity-stream/lib/Store.jsm:140:7

Problem getting stored prefs for TopSites TopSitesFeed.jsm:187
```

So even faking "TRANSACTION_FAILED" exception doesn't cause the reported error………?


AND the sumo reporter just replied back after renaming "storage" directory to "storage.bak":
"Hi philipp, that seems to have fixed it. Thanks for your help, and to everyone else who chipped in."

So.. I guess it is indexeddb related...
(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> Are there particular Telemetry probes we could be looking at to indicate how
> common this issue is for users? Trying to decide how high the priority is
> for when we un-throttle updates.

This is being tracked at https://sql.telemetry.mozilla.org/queries/56200#147276
(In reply to Ed Lee :Mardak from comment #17)
> So even faking "TRANSACTION_FAILED" exception doesn't cause the reported
> error………?

I think I phrased the "not QM/IDB" bit at the start of comment 15 poorly.  I should have said "there's hope this isn't entirely the longstanding problem of profile breakage that results in openDatabase eternally failing, it might not even be our fault!"

QuotaManager and thereby IndexedDB has a number of failure modes in which they rage-quit/eternally fail (which we are working to address and soon will have the engineers for once more).  I tried to synthetically induce these major known failures modes and see how they manifest.  They manifested as INDEXEDDB_OPEN_FAILED rather than TRANSACTION_FAILED.  Hence the hope, because it suggested that the profiles aren't broken.

> AND the sumo reporter just replied back after renaming "storage" directory
> to "storage.bak":
> "Hi philipp, that seems to have fixed it. Thanks for your help, and to
> everyone else who chipped in."

The user mozillification who you are referring to, and who provided the console logs as reported at https://support.mozilla.org/en-US/questions/1223289#answer-1126843 appears to have been suffering from a broken profile.  The error `IndexedDB UnknownErr: ActorsParent.cpp:17599`[1] corresponds to the QuotaManager v1 to v2 upgrade, specifically the IndexedDB upgrade normalization.  That upgrade logic was introduced during Firefox 55, so the user's profile has been very broken ever since that time.

What is especially interesting about this is that user really should be generating "INDEXEDDB_OPEN_FAILED" errors that should show up in telemetry.  But it's possible we're really dealing with 2 types of failures here:
1) Errors are immediately generated and things don't work.
2) Async promises never resolve or reject, so there are no errors reported, but things still don't work.  It's possible that's what was happening in this case.

I think a major question is what type of decimation is telemetry seeing?  Are we getting *ALL* the data-points, or just 10%/other/etc.?

1: https://hg.mozilla.org/releases/mozilla-release/file/tip/dom/indexedDB/ActorsParent.cpp#l17599
(In reply to Andrew Sutherland [:asuth] from comment #19)

> I think a major question is what type of decimation is telemetry seeing? 
> Are we getting *ALL* the data-points, or just 10%/other/etc.?

Yes, we're getting all the data points in the AS telemetry.
(In reply to Andrew Sutherland [:asuth] from comment #19)
> What is especially interesting about this is that user really should be
> generating "INDEXEDDB_OPEN_FAILED" errors that should show up in telemetry. 
Interestingly, I don't see any of these when modifying the query nanj provided:
https://sql.telemetry.mozilla.org/queries/56200#147276

I only see:
highlights_data_late_by_ms
ADDON_INIT_FAILED
topsites_data_late_by_ms
TRANSACTION_FAILED

That got me thinking the exception for `await this._openDatabase();` never happens in the wild, and if I make `createOrOpenDb` await forever, I see the same reported bug of no new tab page and no preferences!

createOrOpenDb is just returning IndexedDB.open:
https://hg.mozilla.org/releases/mozilla-release/file/tip/browser/extensions/activity-stream/lib/ActivityStreamStorage.jsm#l57
Flags: needinfo?(madperson)
Yes, :nanj and I were just coming to the same conclusion on #fx-team in IRC.  From IRC:
6:33 PM <nanj> asuth: i can see quite a lot 'INDEXEDDB_OPEN_FAILED' entries in the logs, but not in the table. looks like the etl didn't insert them to the database due to some data validation failures
6:35 PM <nanj> oh, addon_version is null in this case, etl was not happy with that, it expects some(val) other than null

So this is looking like we're dealing with the QuotaManager rage-quit/eternal failure case after all.  Which is good because it's a known scenario, but bad because it's bad and seems to be happening quite a lot.
Summarizing findings from irc:

IndexedDB.open is failing even after deleting the database. This is due to an underlying broken QuotaManager situation that would require the user to refresh the profile. I would guess fixing QuotaManager will be non-trivial.

Activity Stream fails to load because Store.init awaiting on _initIndexedDB throws and never gets to dispatch(initAction) which tells the rest of Activity Stream to load. https://hg.mozilla.org/releases/mozilla-release/file/tip/browser/extensions/activity-stream/lib/Store.jsm#l155

A narrow fix for Activity Stream is to just keep running without IndexedDB. This currently means snippets blocklist and collapsed sections will not persist. If we allow the rest of Activity Stream to load, we'll any usage of ActivityStreamStorage will result in a TRANSACTION_FAILED event every time unless specially suppressed.

Maria/Tim, is it acceptable to fix and uplift to release 61 something where Activity Stream has snippets blocklist and section collapsed state not persisted across restarts?
Flags: needinfo?(tspurway)
Flags: needinfo?(mpopova)
k88hudson, I believe ASRouter also has some dependency on IndexedDB, but that might just be the snippets blocklist? I believe there's an experiment for it, so analyzing results might need to take this into account?

Are there any other uses of IndexedDB that I might have overlooked if we were to let activity stream run without indexeddb?

1) per-section collapsed state
2) snippets blocklist (used by both SnippetsFeed as ASRouter)
Flags: needinfo?(khudson)
RyanVM, just to be clear, this is something we definitely want to fix/workaround for release? The numbers from comment 14 generated from https://sql.telemetry.mozilla.org/queries/56200#147276 show about ~7k users affected by this so far.
Flags: needinfo?(ryanvm)
andreio, any suggestions on simple fixes that could help activity stream keep running without indexeddb without generating a lot of TRANSACTION_FAILED events when db is a rejected promise?

At minimum a fix would need to do something like the following in Store.jsm's init:

try { await this._initIndexedDB(telemetryKey); } catch (e) {}

That gets activity stream loading for me if I force ActivityStreamStorage._openDatabase to always reject.
Assignee: nobody → andrei.br92
Flags: needinfo?(andrei.br92)
asuth, from irc you said "But we can also maybe have a dot release that has QuotaManager just proactively blow away origins during upgrades/init sweeps."

How complex would this change be? I suppose one consideration is if we could even recover the IndexedDB data instead of just throwing it away.

Also, having QuotaManager getting rid out of the bad state will mean any other IndexedDB users (extensions? websites?) should work again? And Activity Stream probably wouldn't need to uplift a without-indexeddb mode.
Flags: needinfo?(bugmail)
RyanVM, nanj hot fixed an issue that INDEXEDDB_OPEN_FAILED event not showing up. Looks to be about 0.3% of 61 release users hitting this.
Flags: needinfo?(andrei.br92)
Didn't manage to reproduce the reported issue using the hints provided by the affected users. 
- The investigated platforms were Windows (10, 8.1, 7) x86/64, Ubuntu (16.04, 14.04) x86/64 and macOS (10.13. 10.12);
- Also used clean and dirty profiles - with or without installed extensions (included the ones that were mentioned in the reports, too) and various AV solutions (where possible), such as Kaspersky, AVG or Avast, but with no luck;
- The manual (or background) update from older Firefox versions (55-60.0.2) went smoothly, the AS and its preferences specific components not being affected.
Flags: needinfo?(andrei.vaida)
We confirmed that we have and are okay with a narrow fix of just continue loading activity stream with a failed indexeddb, which prevents persisting snippets blocklist and section collapsed states. We'll finish up the github PR and export the change by itself for 62 beta and 61 release uplifts.
Flags: needinfo?(tspurway)
Flags: needinfo?(mpopova)
Flags: needinfo?(khudson)
(In reply to Ed Lee :Mardak from comment #27)
> asuth, from irc you said "But we can also maybe have a dot release that has
> QuotaManager just proactively blow away origins during upgrades/init sweeps."
> 
> How complex would this change be? I suppose one consideration is if we could
> even recover the IndexedDB data instead of just throwing it away.

The fix is complex on some axes and simple on others.  I would characterize the fix as converting existing error handling cases that give up into proactively addressing the problem by deleting collections of files or directories.  That's pretty simple, if a little scary because it does involve permanently erasing data that might hypothetically be manually recovered.  (But it's also clear that we under-estimated the number of users encountering these problems and/or that they would be guided to use the "refresh Firefox" mechanism which 100% addresses the situation.)

It's made a little more complex by the fact that the existing logic is imperative recursive logic whose XPCOM early-return/early-continue control flow may not always make injecting cleanup trivial.  It's made more complex after that because we have a bunch of upgrade logic that tries to bring profiles from the dawn of IndexedDB up to modern times, and because we run them in sequence, there's several logic sites that potentially need to upgrade.  (We'd planned to clean that up and set a hard "your profile must be within the last 2 ESR's" when we had time to clean these things up.)

That said, we can do it.  And thanks to conservative, iterative improvements that have been made, plus the bug reports that have been filed, we have a good idea of where to make the changes and realistic test scenarios to add to our automated tests that can provide confidence that we won't delete too much data.

It sounds like, given comment 31, that perhaps this won't be happening for 61, though?  We'll see about expediting for uplift to 62 perhaps?

> Also, having QuotaManager getting rid out of the bad state will mean any
> other IndexedDB users (extensions? websites?) should work again? And
> Activity Stream probably wouldn't need to uplift a without-indexeddb mode.

The idea is that QuotaManager and IndexedDB should at least operate sufficiently well *for all origins* that indexedDB.deleteDatabase() should at least be usable and recover from any problems so that the next indexedDB.open() for a just-deleted database will recover, which means that ActivityStreams' recovery logic will work reliably (as I tested in my synthetic fault injection at the IndexedDB database level).
Flags: needinfo?(bugmail)
asuth, could you provide a STR for getting IndexedDB/QuotaManager into a bad state (perhaps with a specific file to add/download to the profile directory??)? Comment 30 tried upgrading a profile from 55 and other approaches but could not reproduce. We'll need some way to verify the fix for uplift to release.
Flags: qe-verify+
Flags: needinfo?(bugmail)
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/ce7f00d085f9318335a84efb7b9b8cb8326f1fdc
fix(storage): Catch IndexedDB permafail (#4222)

Bug 1471375 - Reports about missing activity stream content on new tab page and about:preferences#home panel
No longer blocks: 1455216
User Story: (updated)
Iteration: --- → 63.1 - July 9
Priority: -- → P1
User Story: (updated)
Depends on: 1447879
No longer depends on: 1464019
Attached patch beta patchSplinter Review
Attachment #8988800 - Flags: review?(khudson)
Attached patch release patchSplinter Review
Attachment #8988801 - Flags: review?(khudson)
Comment on attachment 8988782 [details]
Bug 1471375 - Reports about missing activity stream content on new tab page and about:preferences#home panel.

https://reviewboard.mozilla.org/r/253966/#review260716
Attachment #8988782 - Flags: review?(khudson) → review+
Attachment #8988800 - Flags: review?(khudson) → review+
Attachment #8988801 - Flags: review?(khudson) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/44ee5fe5de79
Reports about missing activity stream content on new tab page and about:preferences#home panel. r=k88hudson
(In reply to Ed Lee :Mardak from comment #33)
> asuth, could you provide a STR for getting IndexedDB/QuotaManager into a bad
> state (perhaps with a specific file to add/download to the profile
> directory??)? Comment 30 tried upgrading a profile from 55 and other
> approaches but could not reproduce. We'll need some way to verify the fix
> for uplift to release.

To break QuotaManager by simulating a profile from the future:
sqlite3 PROFILE/storage.sqlite "PRAGMA user_version = 262146;"

To un-break QuotaManager:
sqlite3 PROFILE/storage.sqlite "PRAGMA user_version = 131073;"

Both should be done without the profile being in use by Firefox.  (So, quit Firefox, run the command, then start Firefox and see it broken or repaired.)

Pre-requisite: Have sqlite3 on your PATH.  You may need to `sudo apt install sqlite3` if you are linux, and if you're not, you may need a pre-requisite of running Ubuntu or Debian.  SQLite binaries can also be acquired from https://sqlite.org/download.html.


If you would like to simulate a fault in the IndexedDB database which should be recoverable if QuotaManager is not broken, run:
sqlite3 PROFILE/permanent/chrome/idb/1657114595AmcateirvtiSty.sqlite "pragma user_version = 832;"

You don't need to undo that because ActivityStream will delete the database and re-create a new, un-broken one.
Flags: needinfo?(bugmail)
Attached file broken storage.sqlite
JuliaC, could you try to reproduce the problem by first copying the attached storage.sqlite into the profile directory?

Locally, nightly is broken with blank activity-stream but with the patch on autoland, activity stream loads.

For reference, here's what I see in the browser console:
```
IndexedDB UnknownErr: ActorsParent.cpp:600
PushService: stateChangeProcessEnqueue: Error transitioning state UnknownError PushService.jsm:122
IndexedDB UnknownErr: ActorsParent.cpp:600
undefined history-persistence.js:29:5
```
Flags: needinfo?(ryanvm) → needinfo?(iulia.cristescu)
Comment on attachment 8988800 [details] [diff] [review]
beta patch

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream using IndexedDB
[User impact if declined]: Empty home and new tab pages
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]:  Yes, comment 44
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Somewhat
[Why is the change risky/not risky?]: A couple Activity Stream features depend on IndexedDB working, and the fix here is to ignore the IndexedDB failure and let the features be broken-ish. The affected features are relatively minor, cosmetic and low usage, and the team has decided it's an acceptable tradeoff for the ~0.4% of affected users (which when combined with the even low rate of interaction of the affected features should be a small set).
[String changes made/needed]: None.
Attachment #8988800 - Flags: approval-mozilla-beta?
Comment on attachment 8988801 [details] [diff] [review]
release patch

Approval Request Comment 45
Attachment #8988801 - Flags: approval-mozilla-release?
https://hg.mozilla.org/mozilla-central/rev/44ee5fe5de79
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(In reply to Ed Lee :Mardak from comment #44)
> Created attachment 8988880 [details]
> broken storage.sqlite
> 
> JuliaC, could you try to reproduce the problem by first copying the attached
> storage.sqlite into the profile directory?
> 
> Locally, nightly is broken with blank activity-stream but with the patch on
> autoland, activity stream loads.
> 
> For reference, here's what I see in the browser console:
> ```
> IndexedDB UnknownErr: ActorsParent.cpp:600
> PushService: stateChangeProcessEnqueue: Error transitioning state
> UnknownError PushService.jsm:122
> IndexedDB UnknownErr: ActorsParent.cpp:600
> undefined history-persistence.js:29:5
> ```

I managed to reproduce the initial issue thanks to your indications, using 63.0a1 (2018-06-29). I also can confirm that it is not reproducible anymore on 63.0a1 (2018-07-02) across platforms (Windows 10 x64, Ubuntu 18.04 x64, macOS 10.13.4), the AS and its preferences specific components works as expected. One mention, though: 
 Problem getting stored prefs for feeds.section.topstories SectionsManager.jsm:157
 Problem getting stored prefs for feeds.section.highlights SectionsManager.jsm:157
 Problem getting stored prefs for TopSites
are triggered in Browser Console on a profile having broken IndexedDB/QuotaManager and using the fixed 63 build, in addition to the errors previously stated by you. Are these expected?
Flags: needinfo?(iulia.cristescu)
(In reply to Iulia Cristescu, QA [:JuliaC] from comment #48)
>  Problem getting stored prefs for feeds.section.topstories
>  Problem getting stored prefs for feeds.section.highlights
>  Problem getting stored prefs for TopSites
> are triggered in Browser Console on a profile having broken
> IndexedDB/QuotaManager and using the fixed 63 build, in addition to the
> errors previously stated by you. Are these expected?
Yes. Those related features using indexeddb are expecected to not totally function with this fix.
Flags: needinfo?(edilee)
Comment on attachment 8988800 [details] [diff] [review]
beta patch

Allow Activity Stream to remain functional for users with corrupted IndexedDBs. Approved for 62.0b5 and 61.0.1.
Attachment #8988800 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8988801 - Flags: approval-mozilla-release? → approval-mozilla-release+
User Story: (updated)
Thank you, Mardak for your clarification! Per comment 48 and comment 49, Firefox 63 can be marked as verified fixed.
Status: RESOLVED → VERIFIED
an affected user confirmed that the issue got resolved in 62.0b5 too: https://support.mozilla.org/en-US/questions/1223637
(In reply to Andrew Sutherland [:asuth] from comment #11)
> For diagnosing problems, I'd suggest pointing users at
> https://firefox-storage-test.glitch.me/ (note that https is essential).

(In reply to [:philipp] from comment #54)
> an affected user confirmed that the issue got resolved in 62.0b5 too:
> https://support.mozilla.org/en-US/questions/1223637

asuth, interestingly in comment 54's sumo link, the reporter there attached a 61 screenshot of the storage test with all green / Good and "Storage is working" but that same reporter also says the latest beta fixes the problem.

When I locally run the storage test with broken storage.sqlite attachment 8988880 [details], I see "Storage is broken." with only LocalStorage "Good: Totally Working. (fullyOperational)" and QuotaManager, IndexedDB, Cache API "Bad: Totally Broken (fullyBroken)"

So maybe IndexedDB isn't actually completely broken with a failed QuotaManager for these users, and somehow only Activity Stream is breaking? Normandy and RemoteSettings are other Firefox features depending on IndexedDB, and it sounds like they haven't noticed or run into problems that we have here.

In any case, do you have bugs to mark See Also for QuotaManager or IndexedDB followups?
Flags: needinfo?(bugmail)
As a followup, I also can confirm that the fix was successfully applied for 62.0b5 build1 (20180702164905) and 61.0.1 build1 (20180704003137) across platforms (Windows 10 x64, Ubuntu 18.04 x64, macOS 10.13.4), when using the attachment provided in comment 44.
Flags: qe-verify+
See Also: → 1493262
Hi, I'm Tom working on fixing the initialization failure for QuotaManager, IndexedDB with Andrew and Jan.

(In reply to Ed Lee :Mardak from comment #55)
> (In reply to Andrew Sutherland [:asuth] from comment #11)
> > For diagnosing problems, I'd suggest pointing users at
> > https://firefox-storage-test.glitch.me/ (note that https is essential).
> 
> (In reply to [:philipp] from comment #54)
> > an affected user confirmed that the issue got resolved in 62.0b5 too:
> > https://support.mozilla.org/en-US/questions/1223637
> 
> asuth, interestingly in comment 54's sumo link, the reporter there attached
> a 61 screenshot of the storage test with all green / Good and "Storage is
> working" but that same reporter also says the latest beta fixes the problem.
> 
> When I locally run the storage test with broken storage.sqlite attachment
> 8988880 [details], I see "Storage is broken." with only LocalStorage "Good:
> Totally Working. (fullyOperational)" and QuotaManager, IndexedDB, Cache API
> "Bad: Totally Broken (fullyBroken)"
> 
> So maybe IndexedDB isn't actually completely broken with a failed
> QuotaManager for these users, and somehow only Activity Stream is breaking?
> Normandy and RemoteSettings are other Firefox features depending on
> IndexedDB, and it sounds like they haven't noticed or run into problems that
> we have here.

If they are suffering from QuotaManager initialization failure, then their results of storage test should show "Bad: Totally Broken (fullyBroken)" as the result of your testing locally.
  
In this case, I'd say, at least, it's not related to QuotaManager initialization failure. It does seem to be an issue on Activity Stream only or something else.

> In any case, do you have bugs to mark See Also for QuotaManager or IndexedDB
> followups?

We are tracking and fix them in Bug 1482662.
Flags: needinfo?(bugmail)
See Also: → 1482662
Component: Activity Streams: Newtab → New Tab Page
See Also: → 1621301
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: