1442992 - regression: top sites not shown after startup (browser.newtabpage.activity-stream.feeds.section.topstories.options;"")

Status: RESOLVED FIXED

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

Description

[Darkspirit]

Attachment: Screenshot_20180304_150030.png

Top Sites are not immediately shown after startup if browser.newtabpage.activity-stream.feeds.section.topstories.options is set to "" to remove pocket.
I think they appear after a while of using Nightly.

mozregression --good 2018-03-01 --bad 2018-03-03 --pref browser.newtabpage.activity-stream.feeds.section.topstories.options:"" startup.homepage_welcome_url:"about:newtab"
> 5:02.29 INFO: Last good revision: f8688efe482431851107b74a1dd9815f3292a8b7
> 5:02.29 INFO: First bad revision: 930b44b7adc7e26e0b76cb46178489fc2e6704dc
> 5:02.29 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f8688efe482431851107b74a1dd9815f3292a8b7&tochange=930b44b7adc7e26e0b76cb46178489fc2e6704dc

(2018-03-02)
> 930b44b7adc7	Kris Maglione — Bug 1441333: Part 6 - Use caller location in error reports for StrongPromise errors. r=zombie
> cf87433f501d	Kris Maglione — Bug 1441333: Part 5 - Use proper async caller location in normalizeError. r=zombie
> 47c2e679000f	Kris Maglione — Bug 1441333: Part 4 - Add caller location to async API errors. r=zombie
> c21a778bab3d	Kris Maglione — Bug 1441333: Part 3 - Add helper to create a JS error with a saved stack. r=bz
> 61f204267373	Kris Maglione — Bug 1441333: Part 2 - Allow passing an error stack to reportError. r=bz
> f9eb113f179f	Kris Maglione — Bug 1441333: Part 1 - Add helper to retrieve closest stack frame with a given principal. r=bz
> 4e681b986b5c	Kris Maglione — Bug 1440816: Part 1 - Set readPrincipals callback on JS context. r=fitzgen

I don't know which one of those two bugs caused this regression. Please remove the other one.

Comment 1

[Darkspirit]

browser.newtabpage.activity-stream.feeds.section.topstories.options;{} is fine.

Comment 2

[Kris Maglione [:kmag]]

This is almost certainly unrelated to those changes. They only affect the way we report errors, and should only have any observable effect for WebExtensions (which Activity Streams is not). If it affects them in any way, it would suggest that they're doing something very, very wrong.

Comment 3

[Darkspirit]

Maybe mozregression was confused by some backouts? At the beginning, when I ran mozregression without that activity-stream pref I noticed that there were some broken builds.

Comment 4

[Kris Maglione [:kmag]]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #3)
> Maybe mozregression was confused by some backouts? At the beginning, when I
> ran mozregression without that activity-stream pref I noticed that there
> were some broken builds.

That's certainly possible.

Comment 5

[Andrei Oprea [:andreio]]

Attachment: Screenshot from 2018-03-05 10-59-42.png

I wasn't able to reproduce the Topsites loading failures in today's m-c, only this weird failure state of the section when option parsing fails.
Can you please confirm?

Comment 6

[Darkspirit]

Attachment: Screenshot_20180305_135132.png

mozregression --launch 20180305100134 --pref browser.newtabpage.activity-stream.feeds.section.topstories.options:"" startup.homepage_welcome_url:"about:newtab"

Comment 7

[Andrei Oprea [:andreio]]

Attachment: Screenshot from 2018-03-05 15-34-18.png

If you pass in "" in mozregression when you call Services.prefs.getStringPref you will get back `""""`. Our pref parser has default argument value of "{}" so if you just pass --pref pref_name= (leave blank) it works as expected.

Comment 8

[Darkspirit]

> mozregression --launch 20180305100134 --pref browser.newtabpage.activity-stream.feeds.section.topstories.options= startup.homepage_welcome_url:"about:newtab"

doesn't modify browser.newtabpage.activity-stream.feeds.section.topstories.options on about:config.

> mozregression --launch 20180305100134 --pref browser.newtabpage.activity-stream.feeds.section.topstories.options: startup.homepage_welcome_url:"about:newtab"

would be an empty string (like --pref name:"") and looks the same as comment 6.
The regression is that it's no longer able to handle an empty value.

> mozregression --launch 20180305100134 --pref browser.newtabpage.activity-stream.feeds.section.topstories.options:"{}" startup.homepage_welcome_url:"about:newtab"

works as expected. It's just {} as value on about:config.

Comment 9

[Andrei Oprea [:andreio]]

Sorry looking more at this there might be something related to the bugs you initially reported. We use `Cu.reportError` in the catch branch where we parse the preferences [0]. It looks like Cu.reportError causes the function to return early. kmag is this expected?

[0] https://github.com/mozilla/activity-stream/blob/d4c5fa7eb6f54b29f08069b313ffa0a8556eca7a/system-addon/lib/SectionsManager.jsm#L119

Comment 10

[Kris Maglione [:kmag]]

Ah. I see. You're passing a second argument to Cu.reportError. That argument used to be ignored. Now it is used, and needs to be a SavedStack it preaent.

Comment 11

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4021

Comment 12

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/138063d4668b8705da3b46ab2590c575e0af8534
Fix Bug 1442992 - Remove second argument from Cu.reportError call.

Comment 13

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/9e902a91c375