Closed Bug 1249702 Opened 4 years ago Closed 4 years ago

[e10s] Feed preview hangs browser when site content handler set

Categories

(Firefox Graveyard :: RSS Discovery and Preview, defect)

defect
Not set

Tracking

(e10sm9+, firefox44 unaffected, firefox45 wontfix, firefox46+ fixed, firefox47+ fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
e10s m9+ ---
firefox44 --- unaffected
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Keywords: regression)

Attachments

(2 files)

STR:

1. Use a Firefox version from several years ago (unsure on which so far)
2. Go to newsblur.com
3. Click "Try out NewsBlur"
4. Click Gear Icon in left side panel -> Goodies & Mobile Apps
5. Click button next to "Firefox: Register NewsBlur as an RSS reader"
6. Click "Add Feed Reader" in the Firefox notification bar that appears
7. In about:config, some prefs should now be set, including:

browser.contentHandlers.auto.application/vnd.mozilla.maybe.feed = https://newsblur.com/?url=%s

(This pref seems to no longer be set during these steps in recent Firefox.)

8. Open the same profile in Firefox 46 or later, with e10s enabled.
9. Go to any feed URL, such as http://chicago.curbed.com/atom.xml

ER:

I expect... anything better than what is below. :)

AR:

The Firefox feed preview UI appears, but it will hang the browser UI for 30 - 60 secs.  Also, the feed UI is just an empty shell and never shows feed content.

Also, the browser console says "TypeError: this._feedWriter is null".

Notes:

So, clearing the pref fixes the hang... should we clear such prefs for everyone?
I suppose this regressed with bug 1109714?
Oh!  There are actually more errors:

TypeError: this._feedWriter is null subscribe.js:21:5
too much recursion WebContentConverter.js:884:20
NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "too much recursion" {file: "jar:file:///Applications/FirefoxNightly.app/Contents/Resources/browser/omni.ja!/components/WebContentConverter.js" line: 884}]'[JavaScript Error: "too much recursion" {file: "jar:file:///Applications/FirefoxNightly.app/Contents/Resources/browser/omni.ja!/components/WebContentConverter.js" line: 884}]' when calling method: [nsIWebContentConverterService::getContentHandlers] FeedWriter.js:815:0
uncaught exception: out of memory <unknown>
TypeError: this._feedWriter is null subscribe.js:17:5
[Tracking Requested - why for this release]:
Blocks: 1109714
If it's bug 1109714, it should work correctly in beta, can you confirm that?
[Tracking Requested - why for this release]:

(In reply to Marco Bonardo [::mak] from comment #4)
> If it's bug 1109714, it should work correctly in beta, can you confirm that?

44 (release) works, but 45 (beta) also fails.  It looks like bug 1109714 was uplifted to 45, so it seems like the right suspect.
Attached patch feeds-hangSplinter Review
Here is a WIP that at least fixes the page to stop hanging.

However, there is still an issue in Feeds.jsm where it gives the error:

uncaught exception: 2147746065 Feeds.jsm:50:9
NS_ERROR_NOT_AVAILABLE: Component is not available'Component is not available' when calling method: [nsIWebContentConverterService::setAutoHandler]
Attachment #8721443 - Flags: feedback?(mak77)
Gijs, maybe you can provide feedback?  It seems like we should try to land something soon, if we want to fix 45.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8721443 [details] [diff] [review]
feeds-hang

Review of attachment 8721443 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, I can take over reviews/feedbacks; Marco is swamped with sanitize.js issues on beta.

::: browser/components/feeds/WebContentConverter.js
@@ +881,5 @@
>    _init() {
> +    if (this._initing) {
> +      return;
> +    }
> +    this._initing = true;

Do you have a stack for the recursive initialization that we're hitting here? Seems like we should fix the cause rather than the symptoms...

@@ +921,5 @@
>          let uri = autoBranch.getCharPref(type);
>          if (uri) {
>            let handler = this.getWebContentHandlerByURI(type, uri);
>            if (handler) {
> +            this.setAutoHandler(type, handler);

This doesn't seem correct to me off-hand. The NS_ERROR_NOT_AVAILABLE issue is caused by this change, as the not-underscored version of this method checks the type against the _contentTypes on this object, which the _-prefixed one doesn't. I'm not clear on why we need to use the unprefixed version here, can you clarify why you're making this change?

::: browser/modules/Feeds.jsm
@@ +45,5 @@
>        }
>  
>        case "WCCR:setAutoHandler": {
>          let registrar = Cc["@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"].
> +                          getService(Ci.nsIWebContentConverterService);

r=me for this bit for sure. Is that enough to fix the hanging?
Attachment #8721443 - Flags: feedback?(mak77)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> [Tracking Requested - why for this release]:
> 
> (In reply to Marco Bonardo [::mak] from comment #4)
> > If it's bug 1109714, it should work correctly in beta, can you confirm that?
> 
> 44 (release) works, but 45 (beta) also fails.  It looks like bug 1109714 was
> uplifted to 45, so it seems like the right suspect.

How hard would it be to back 1109714 out from beta? It seems like it's no use any more as we won't ship e10s there anyway, and if it's breaking things in non-e10s then that's just a net regression that we should kill off.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(jryans)
(In reply to :Gijs Kruitbosch from comment #8)
> ::: browser/components/feeds/WebContentConverter.js
> @@ +881,5 @@
> >    _init() {
> > +    if (this._initing) {
> > +      return;
> > +    }
> > +    this._initing = true;
> 
> Do you have a stack for the recursive initialization that we're hitting
> here? Seems like we should fix the cause rather than the symptoms...

Here's a stack recorded by logging the second time we enter _init:

_init@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/WebContentConverter.js:883:11
getContentHandlers@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/WebContentConverter.js:984:5
getWebContentHandlerByURI@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/WebContentConverter.js:1005:12
_init@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/WebContentConverter.js:924:25
getContentHandlers@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/WebContentConverter.js:984:5
_initSubscriptionUI@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/FeedWriter.js:815:20
init@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/FeedWriter.js:938:5
SH_init@chrome://browser/content/feeds/subscribe.js:13:24

If we have the special pref set from the auto branch (PREF_CONTENTHANDLERS_AUTO), then _init will call this.getWebContentHandlerByURI, which calls this.getContentHandlers, which calls _init again.

> @@ +921,5 @@
> >          let uri = autoBranch.getCharPref(type);
> >          if (uri) {
> >            let handler = this.getWebContentHandlerByURI(type, uri);
> >            if (handler) {
> > +            this.setAutoHandler(type, handler);
> 
> This doesn't seem correct to me off-hand. The NS_ERROR_NOT_AVAILABLE issue
> is caused by this change, as the not-underscored version of this method
> checks the type against the _contentTypes on this object, which the
> _-prefixed one doesn't. I'm not clear on why we need to use the unprefixed
> version here, can you clarify why you're making this change?

There are two implementations of similar interfaces inside WebContentConverter.js, there is both WebContentConverterRegistrar and WebContentConverterRegistrarContent.  The block at the end of the file chooses which to use based on process type.

The problematic, infinite looping _init method is part of WebContentConverterRegistrarContent, which only defines setAutoHandler right now, so calling this._setAutoHandler is an error.

(I am really not sure what's **supposed** to happen... I just attempting the most straightforward thing I could think of.)  Still not sure of the correct fix for the NS_ERROR_NOT_AVAILABLE issue, any help would be great!  Maybe we can talk through it on IRC?

> 
> ::: browser/modules/Feeds.jsm
> @@ +45,5 @@
> >        }
> >  
> >        case "WCCR:setAutoHandler": {
> >          let registrar = Cc["@mozilla.org/embeddor.implemented/web-content-handler-registrar;1"].
> > +                          getService(Ci.nsIWebContentConverterService);
> 
> r=me for this bit for sure. Is that enough to fix the hanging?

No, this piece is not even triggered until you change to `setAutoHandler` instead of `_setAutoHandler`.  The hang is the infinite loop in _init.

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> > [Tracking Requested - why for this release]:
> > 
> > (In reply to Marco Bonardo [::mak] from comment #4)
> > > If it's bug 1109714, it should work correctly in beta, can you confirm that?
> > 
> > 44 (release) works, but 45 (beta) also fails.  It looks like bug 1109714 was
> > uplifted to 45, so it seems like the right suspect.
> 
> How hard would it be to back 1109714 out from beta? It seems like it's no
> use any more as we won't ship e10s there anyway, and if it's breaking things
> in non-e10s then that's just a net regression that we should kill off.

Seems complex to back out, but we could try it...  it was quite a large patch to accept for uplift to 45 (Dev. Ed. at the time) in the original bug.  However, the hang only occurs with e10s, so if we aren't using e10s with 45 in Release, then maybe we don't need to worry as much about 45.
Flags: needinfo?(jryans) → needinfo?(gijskruitbosch+bugs)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > ::: browser/components/feeds/WebContentConverter.js
> > @@ +881,5 @@
> > >    _init() {
> > > +    if (this._initing) {
> > > +      return;
> > > +    }
> > > +    this._initing = true;
> > 
> > Do you have a stack for the recursive initialization that we're hitting
> > here? Seems like we should fix the cause rather than the symptoms...
> 
> Here's a stack recorded by logging the second time we enter _init:
> 
> _init@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/
> dist/Nightly.app/Contents/Resources/browser/components/WebContentConverter.
> js:883:11
> getContentHandlers@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-
> firefox-release/dist/Nightly.app/Contents/Resources/browser/components/
> WebContentConverter.js:984:5
> getWebContentHandlerByURI@file:///Users/jryans/projects/mozilla/gecko-dev-2/
> obj-firefox-release/dist/Nightly.app/Contents/Resources/browser/components/
> WebContentConverter.js:1005:12
> _init@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/
> dist/Nightly.app/Contents/Resources/browser/components/WebContentConverter.
> js:924:25
> getContentHandlers@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-
> firefox-release/dist/Nightly.app/Contents/Resources/browser/components/
> WebContentConverter.js:984:5
> _initSubscriptionUI@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-
> firefox-release/dist/Nightly.app/Contents/Resources/browser/components/
> FeedWriter.js:815:20
> init@file:///Users/jryans/projects/mozilla/gecko-dev-2/obj-firefox-release/
> dist/Nightly.app/Contents/Resources/browser/components/FeedWriter.js:938:5
> SH_init@chrome://browser/content/feeds/subscribe.js:13:24

OK, based on that, I guess this makes sense. Add a comment above it maybe, to indicate that we could be called recursively because we call some of our own methods, and those call init() ? Although actually...

> (I am really not sure what's **supposed** to happen... I just attempting the
> most straightforward thing I could think of.)  Still not sure of the correct
> fix for the NS_ERROR_NOT_AVAILABLE issue, any help would be great!  Maybe we
> can talk through it on IRC?

I think the autobranch stuff at the bottom of init() shouldn't run at all in the content version of the thing. It doesn't have any code for the auto client handlers (ie, there's no way to check what auto handlers are registered anyway!), like the parent has, and the parent runs identical init() code on its side of things - but the content/child has no way of getting that information from the parent right now, and sending it up to the parent is useless, because the parent does it itself anyway. So I think we can just remove the auto branch stuff from the content version of the content-side init(). That will also fix the infinite looping issue.

> However, the hang only occurs with e10s, so if we aren't using e10s with 45
> in Release, then maybe we don't need to worry as much about 45.

OK, then let's not worry about 45, indeed.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Comment on attachment 8722541 [details]
MozReview Request: Bug 1249702 - Fix e10s feed handling with auto content handler prefs. r=Gijs

https://reviewboard.mozilla.org/r/36089/#review32759
Attachment #8722541 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/1bcb9055be8c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment on attachment 8722541 [details]
MozReview Request: Bug 1249702 - Fix e10s feed handling with auto content handler prefs. r=Gijs

Approval Request Comment
[Feature/regressing bug #]: e10s + feed reading
[User impact if declined]: infinite loop / hang in some cases
[Describe test coverage new/current, TreeHerder]: minimal test coverage for this feature, sadly
[Risks and why]: low. The patch is simple. The only potential downside is that the prefs that cause the hang here would now not be applied. I do not believe that is actually the case, but it's theoretically possible. I would argue that that is still a strict improvement over hanging completely if such prefs are present. :-)
[String/UUID change made/needed]: nope
Attachment #8722541 - Flags: approval-mozilla-aurora?
Marking 45 as wontfix.  The issue is present there, but only manifests with e10s on, and we don't plan to enable e10s when 45 is on release.
Comment on attachment 8722541 [details]
MozReview Request: Bug 1249702 - Fix e10s feed handling with auto content handler prefs. r=Gijs

Sounds good to get feeds working with e10s. Please uplift to aurora.
Attachment #8722541 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking since this is regression when we ship e10s.
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.