1183296 - RSS and Atom feeds not displayed correctly in E10S tabs

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Hoye [:mhoye]]

On OSX nightly, opening an RSS feed in an e10s tab displays the usual "subscribe" header plus an empty rectangle with a single line in it, as here:

http://imgur.com/SKt06Ry

Console says "uncaught exception: out of memory". 

This happens with any RSS feed I've tried, in both nightly and Dev Edition, it does not occur in non-e10s tabs, and in conversation gw280 doesn't think it's the same as bug 1109714.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey jimicy, can you please braindump your findings in here?

Comment 2

[Alistair Buxton]

Also affects Linux 64 bit.

https://bugzilla.mozilla.org/show_bug.cgi?id=1193945 is probably the same bug.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Jimmy, will you keep working on these?

Comment 4

[Jimmy Wang (:jimicy) - works on e10s stuff]

Hi Brad,
I won't continue working on this bug. Here are my findings though.

TypeError: this._feedWriter is null subscribe.js:17:5
This happens since you can't make do new BrowserFeedWriter() since the code in FeedWriter.js errors out.

if you comment this line out in FeedWriter.js, you will be able to see rss feeds
this._initSubscriptionUI();

The code inside this_initSubscriptionUI(...) causing the problem is

uncaught exception: 2147500034
NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]

caused by

> // List of web handlers
>     var wccr = Cc["@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"].
>                getService(Ci.nsIWebContentConverterService);

NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPrefBranch.setBoolPref]
caused by

> prefs.setBoolPref(PREF_SHOW_FIRST_RUN_UI, false);

I have attached a patch called bug1183296-commented-code with the erroring code parts I mentioned commented out in FeedWriter.js

---

browser/components/feeds/FeedWriter.js is registering itself as a XPCOM component that gets loaded in the content process for e10s. However it does prefs.setBoolPref(PREF_SHOW_FIRST_RUN_UI, false); which can't be done from the content process. In fact the other pref setting code in FeedWriter.js might not be erroring out, but they shouldn't there since you can't set prefs in the content process.

Move prefs code and other parent process code from FeedWriter.js to nsBrowserGlue.js
To send a message from FeedWriter.js to nsBrowserGlue.js in the parent process use

in FeedWriter.js
> let mm = aWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDocShell).QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIContentFrameMessageManager);

all the tests we have are in http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/test/

Comment 5

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: bug1183296-commented-code

Comment 6

[Gervase Markham [:gerv]]

I also see this on Linux.

Gerv

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Tracking for 42 as it is a spotify is an important website.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Steps to reproduce & regression range outlined in bug 1193945.

Comment 11

[George Wright (:gw280) (needinfo me!)]

I'll take this back.

Comment 12

[Hussam Al-Tayeb]

Just to confirm that this is the correct bug that I am hitting, is the "subscribe now" button also broken?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Not tracking it anymore as we are going to disable e10s soon.

Comment 16

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm

Bug 1183296 - feeds should display in e10s mode, r?mconley

Comment 17

[:Gijs (he/him)]

Stealing!

I didn't bother fixing the web content something or other handler thing. This at least gets us back visible feed pages. The other bug (about the subscribe UI being busted) can take care of the subscribe UI, I think, and we can file a follow up for web feed services being busted...

I hand-hacked a pref forwarding thing, but if we're stopping pref sets from the content process, it seems like it might make sense to stick that in an independent module that can be included from anywhere?

Comment 18

[:Gijs (he/him)]

(other bug being bug 1109714)

Comment 19

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

Comment on attachment 8659907 [details]
MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm

https://reviewboard.mozilla.org/r/19029/#review16945

::: browser/base/content/browser-feeds.js:183
(Diff revision 1)
> +  _setPref(prefType, prefName, prefValue, prefComplexValue) {

This API seems overly broad and I'm worried it will allow people to break out of our content process sandbox. If we assume the content process has been exploited, it seems like someone could set a dangerous pref and get additional access. The infamous security.turn_off_all_security_so_that_viruses_can_take_over_this_computer option (possibly now removed because of its used in several Pwn2Own competitions) comes to mind.

::: browser/components/feeds/FeedWriter.js:1034
(Diff revision 1)
> +    if (Services.appinfo.processType != Services.appinfo.PROCESS_TYPE_CONTENT) {

Can you put a FIXME with a bug number here?

::: browser/components/feeds/FeedWriter.js:1256
(Diff revision 1)
> -        prefs.setCharPref(getPrefReaderForType(feedType), "web");
> +        this._proxyPrefSet("Char", getPrefReaderForType(feedType), "web");

It seems like we're going to need to move more of this code into the parent process to actually get it working. The file picker doesn't work well enough in the content process to be usable here for picking an application. (It's really only designed for <input type=file>.)

I would kind of prefer to leave off the pref changes from this patch. Whoever ends up fixing the subscribe UI can then take care of that. I suspect we'll want to move a lot more of it to the parent.

If you think there's value to this code by itself, though, we can discuss it some more.

Comment 22

[:Gijs (he/him)]

Stealing per discussion on IRC.

Comment 23

[:Gijs (he/him)]

Comment on attachment 8659907 [details]
MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm

Bug 1183296 - feeds should display in e10s mode, r?billm

Comment 24

[:Gijs (he/him)]

Per discussion with George, this patch just enables display to work (and the first run UI because I already had the infrastructure to do that easily).

The subscribe UI can be dealt iwth in bug 1109714, which George marked as a dep.

Comment 25

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

Comment on attachment 8659907 [details]
MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm

https://reviewboard.mozilla.org/r/19029/#review17989

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/30d2d8bdf0ba

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30d2d8bdf0ba

Comment 28

[Mike Hoye [:mhoye]]

Thanks everyone.

Comment 29

[:Gijs (he/him)]

Comment on attachment 8659907 [details]
MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: no feed display when e10s is on on devedition/aurora
[Describe test coverage new/current, TreeHerder]: limited test coverage
[Risks and why]: low risk, these are just some minor tweaks to make the basics work here
[String/UUID change made/needed]: nope

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8659907 [details]
MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm

Approved for uplift to aurora, let's keep rss working!

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5bacade79fc5