RSS and Atom feeds not displayed correctly in E10S tabs

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mhoye, Assigned: Gijs)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm8+, firefox42- wontfix, firefox43+ fixed, firefox44 fixed)

Details

Attachments

(2 attachments)

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.
Assignee: nobody → gwright
Assignee: gwright → jimmyw22
Hey jimicy, can you please braindump your findings in here?
Flags: needinfo?(jimmyw22)
Also affects Linux 64 bit.

https://bugzilla.mozilla.org/show_bug.cgi?id=1193945 is probably the same bug.
Jimmy, will you keep working on these?
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/
Flags: needinfo?(jimmyw22)
Assignee: jimmyw22 → nobody
I also see this on Linux.

Gerv
Summary: RSS feeds not displayed correctly in E10S tabs on OSX → RSS and Atom feeds not displayed correctly in E10S tabs
Duplicate of this bug: 1198992
Tracking for 42 as it is a spotify is an important website.
Duplicate of this bug: 1193945
Steps to reproduce & regression range outlined in bug 1193945.
I'll take this back.
Assignee: nobody → gwright
Just to confirm that this is the correct bug that I am hitting, is the "subscribe now" button also broken?
Not tracking it anymore as we are going to disable e10s soon.
Duplicate of this bug: 1203409
Duplicate of this bug: 1203069
Bug 1183296 - feeds should display in e10s mode, r?mconley
Attachment #8659907 - Flags: review?(wmccloskey)
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?
(other bug being bug 1109714)
See Also: → 1109714
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.
Attachment #8659907 - Flags: review?(wmccloskey)
Duplicate of this bug: 1194585
Duplicate of this bug: 1205755
Stealing per discussion on IRC.
Assignee: gwright → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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
Attachment #8659907 - Attachment description: MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?mconley → MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm
Attachment #8659907 - Flags: review?(wmccloskey)
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 on attachment 8659907 [details]
MozReview Request: Bug 1183296 - feeds should display in e10s mode, r?billm

https://reviewboard.mozilla.org/r/19029/#review17989
Attachment #8659907 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/mozilla-central/rev/30d2d8bdf0ba
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Thanks everyone.
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
Attachment #8659907 - Flags: approval-mozilla-aurora?
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!
Attachment #8659907 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1213204
You need to log in before you can comment on or make changes to this bug.