1046500 - Some rss feeds (live bookmarks) fail to load in Firefox 32 and newer

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[custom.firefox.lady [:tawn]]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:33.0) Gecko/20100101 Firefox/33.0 (Beta/Release)
Build ID: 20140729004006

Steps to reproduce:

Go to http://feeds.foxnews.com/foxnews/scitech?format=xml
Click 'Subscribe Now' (using Live Bookmarks)
Try to view the feed you just subscribed to


Actual results:

Feed is empty with the error 'Live Bookmark feed failed to load.


Expected results:

Live mark items should appear in the list just like they did in Firefox 31 and older.
(Some other fox news feeds have the same issue, others such as http://feeds.foxnews.com/foxnews/health?format=xml load just fine.

Comment 1

[[:Aleksej]]

g 2014-04-29-03-02-01-mozilla-central-firefox-32.0a1.en-US.linux-x86_64 d7c07694f339
b 2014-04-30-03-02-01-mozilla-central-firefox-32.0a1.en-US.linux-x86_64 e19812f56952
b 2014-08-04-03-02-05-mozilla-central-firefox-34.0a1.en-US.linux-x86_64

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d7c07694f339&tochange=e19812f56952

Maybe
253acf385229	Wes Johnston — Bug 998063 - Support media:thumbnail in feed parser. r=mak

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

There was some duplicated logic in FeedParser that I tried to clean up, but mangled on the way. This unmangles it a bit. Looking at tests...

Alternatively, we could back out my change on Beta/Aurora.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Tests Patch

It looks like we checked in a test framework here that was meant to run locally, but never integrated into automation. Or maybe it uses a different harness than I'm used to? I ported it over to xpcshell this morning and verified it would have caught this. I just assumed it was running before :(

Pushed this to try:
https://tbpl.mozilla.org/?tree=Try&rev=0404d6dc6116

I also noticed that the presence of a thumbnail and a video makes us interpret a feed as a FEED that previously was detected as a VIDEO_FEED. I'm not sure what the right behaviour is there, but it doesn't disagree with what's written here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/feeds/FeedProcessor.js#312

Comment 4

[Phil Ringnalda (:philor)]

What info did you need?

Where do they run? As part of make check. What broke causing a FAIL, like the two permafailing mrss tests (look for "FAIL |" in any supposedly green desktop build log), to not be noticed as a test failure? Dunno, but I'd put my first bet on the creation of a separate cppunittest suite.

Why do they run in make check, rather than a harness? Guessing based on things he said about other similar situations, despite having probably written more of our test harnesses than any other single person sayrer never really like them and thought all tests should run considerably closer to bare metal than we tend to.

If every item in a feed contains a video enclosure, and also contains an image enclosure, should it be VIDEO_FEED? Yep, it should be.

Is the VIDEO_FEED distinction one which should be carefully maintained? Only if there's some current name for video podcasts|vidcasts|vcasts and clients for them which I can't find. The theory behind it was that you would have one feed reader that you automatically sent feeds for reading to, and then a different podcatcher that you send AUDIO_FEEDs to, and a third video client that you sent VIDEO_FEEDs to, but all I can find in the current market are audio podcatchers that also do video, and audio podcatchers that either don't do video, or don't mention whether or not they do video (absolutely no idea what we thought IMAGE_FEEDs would be, or what would consume them, or whether we purely invented the entire idea of such a thing just because mimetypes starting with image/ exist, but the latter seems most likely). Were someone to ask me how I think they should implement it for the current world, I would say "if every item has at least one video or audio attachment, it's a MEDIA_FEED, otherwise it's a FEED." Making that change would sort of suck (one time) for people who've already saved an automatic choice for VIDEO_FEED and AUDIO_FEED, but it would remove the suck of "I already said to automatically subscribe in Foo Reader, why is this feed with an image attachment on every item showing me the preview page?" and "I automatically send podcasts to iTunes, why does this one with a thumbnail not go there?"

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 8467312 [details] [diff] [review]
Tests Patch

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

You should also remove references to shell.js from toolkit/components/feeds/Makefile.in, that means very likely you can remove Makefile.in completely
You should also remove test.js, nobody will be sad about that.

please fix coding style for "if ()" and "while ()", most are missing the whitespace as "if(".

I'm sorry you had to go through this, the more I look at this code the more I feel bad for you!

::: toolkit/components/feeds/test/shell.js
@@ +20,4 @@
>  //              |
>  //              -- atom/testcase.xml
>  
>  function trimString(s)

likely this predates String.prototype.trim(), or someone didn't like JS :)
Please remove and use .trim()

@@ +37,2 @@
>  
> +    var parser = Components.classes["@mozilla.org/feed-processor;1"]

please define Cc and Ci at the top of this head file and use them.

@@ +71,5 @@
> +TestListener.prototype = {
> +  handleResult: function(result){
> +    var feed = result.doc;
> +    try {
> +      // QI to something

what?

@@ +72,5 @@
> +  handleResult: function(result){
> +    var feed = result.doc;
> +    try {
> +      // QI to something
> +      ok(true, "Testing feed " + this.testcase.file.path);

do_print()

@@ +88,5 @@
> +    run_next_test();
> +  }
> +}
> +
> +const nsIDirectoryServiceProvider = Components.interfaces.nsIDirectoryServiceProvider;

not useful, please remove

@@ +89,5 @@
> +  }
> +}
> +
> +const nsIDirectoryServiceProvider = Components.interfaces.nsIDirectoryServiceProvider;
> +const nsIDirectoryServiceProvider_CONTRACTID = "@mozilla.org/file/directory_service;1";

ditto

@@ +90,5 @@
> +}
> +
> +const nsIDirectoryServiceProvider = Components.interfaces.nsIDirectoryServiceProvider;
> +const nsIDirectoryServiceProvider_CONTRACTID = "@mozilla.org/file/directory_service;1";
> +const ioService = Components.classes['@mozilla.org/network/io-service;1'].getService(Components.interfaces.nsIIOService);

please Cu.import NetUtil.jsm and use NetUtil.newURI instead of iosvc

@@ +93,5 @@
> +const nsIDirectoryServiceProvider_CONTRACTID = "@mozilla.org/file/directory_service;1";
> +const ioService = Components.classes['@mozilla.org/network/io-service;1'].getService(Components.interfaces.nsIIOService);
> +
> +function parseFile(test) {
> +  test.QueryInterface(Components.interfaces.nsILocalFile);

I think this should be done outside and parseFile should already get a QI-ed testFile argument

@@ +95,5 @@
> +
> +function parseFile(test) {
> +  test.QueryInterface(Components.interfaces.nsILocalFile);
> +
> +  if(test.exists() && test.isFile()) {

ditto, should be done outside

@@ +100,5 @@
> +    // Got a feed file, now we need to add it to our tests
> +    // and parse out the Description and Expect headers.
> +    // That lets us write one test.js file and keep the
> +    // tests with the XML. Convenient method learned from
> +    // Mark Pilgrim.

please rewrap at ~80 chars
Also keep only the first phrase, the second/third refer to test.js and we're getting rid of that.

@@ +102,5 @@
> +    // That lets us write one test.js file and keep the
> +    // tests with the XML. Convenient method learned from
> +    // Mark Pilgrim.
> +    var istream = Components.classes["@mozilla.org/network/file-input-stream;1"]
> +                            .createInstance(Components.interfaces.nsIFileInputStream);

As a side note, I think all of this code (both directory walking and file reading) would be far more readable using OS.File ans Task.jsm... though I figure that would be an almost complete rewrite and I don't plan to put that weight on you. Feel free to leave it as-is (nsIFile) or if you're brave move it to OS.File.

@@ +113,5 @@
> +      do {
> +        hasmore = istream.readLine(line);
> +        descIndex = line.value.indexOf('Description:');
> +
> +        if(descIndex > -1) {

descIndex is only used here, so it's not a useful var, just inline the condition

@@ +118,5 @@
> +          testcase.desc = trimString(line.value.substring(line.value.indexOf(':')+1));
> +        }
> +
> +        expectIndex = line.value.indexOf('Expect:');
> +        if(expectIndex > -1) {

ditto, expectIndex is pointless

@@ +123,5 @@
> +          testcase.expect = trimString(line.value.substring(line.value.indexOf(':')+1));
> +        }
> +
> +        baseIndex = line.value.indexOf('Base:');
> +        if(baseIndex > -1) {

ditto

@@ +164,5 @@
> +  // find the directory containing our test XML
> +
> +  if (0 < arguments.length) {
> +
> +    // dir is specified on the command line

I don't think we need to support this case anymore.

@@ +174,5 @@
> +  } else {
> +    var dirServiceProvider = Components.classes[nsIDirectoryServiceProvider_CONTRACTID]
> +        .getService(nsIDirectoryServiceProvider);
> +    var persistent = new Object();
> +    topDir = dirServiceProvider.getFile("CurWorkD", persistent);

let cwd = do_get_cwd();

@@ +177,5 @@
> +    var persistent = new Object();
> +    topDir = dirServiceProvider.getFile("CurWorkD", persistent);
> +  }
> +
> +  var entries = topDir.directoryEntries;

we already know there is an "xml" folder here, so I think you should not check that, just append "xml" to cwd and you have your folder.
and at this point since the code would be much simpler I'd say parseDir can be inlined directly.

::: toolkit/components/feeds/test/xpcshell.ini
@@ +1,3 @@
> +[DEFAULT]
> +head = 
> +tail = 

trailing spaces

I think I'd prefer if this would have a more common structure, so that most of shell.js (shared utils) are in a head.js file, while run_test is in a test_xml.js file... also because seeing a tbpl failure in a file called shell.js doesn't really help associating it to RSS handling :)

Comment 6

[Wesley Johnston (:wesj)]

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

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8467207 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 998063
[User impact if declined]: Some feeds will not load in Firefox
[Describe test coverage new/current, TBPL]: These tests run in automation, but not in a way that made these failures show on TBPL. I'm fixing them here to run in the normal xpcshell harness.
[Risks and why]: Low risk. The feed parser isn't used often and this only affects some feeds. The patch should just restore old behavior. I've verified the tests failed when run locally and all pass with this fix applied.
[String/UUID change made/needed]: None.

Comment 8

[Marco Bonardo [:mak]]

are you going to manage the tests elsewhere? otherwise this is a leave-open

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d5dd62970ae8

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Tests

Updated more. I started to move to OS.File and realized I didn't know what I was doing very well. While I'd love to learn, I figured it was a good follow up bug. Pushed these to try:
https://tbpl.mozilla.org/?tree=Try&rev=aefa2fee90c1

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8470327 [details] [diff] [review]
Tests

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

You didn't remove the Makefile, thus the try run is failing in make check trying to find shell.js

it is much better now! I mostly have some coding style comments and minor things.

r=me with a green Try.

::: toolkit/components/feeds/test/head.js
@@ +18,5 @@
> +    do {
> +      var line = {};
> +      hasmore = istream.readLine(line);
> +
> +      if(line.value.indexOf('Description:') > -1) {

please fix the "if(" styling to "if ("

this happens multiple times here, not commenting further

@@ +27,5 @@
> +        testcase.expect = line.value.substring(line.value.indexOf(':')+1).trim();
> +      }
> +
> +      if(line.value.indexOf('Base:') > -1) {
> +        testcase.base = NetUtil.newURI(line.value.substring(line.value.indexOf(':')+1).trim(),null,null);

there's no need to pass null,null to NetUtil.newURI, they are optional

@@ +39,5 @@
> +
> +    } while (hasmore);
> +
> +  } catch(e) {
> +    ok(false, "FAILED! Error reading testFile case in file " + testFile.leafName  + " ---- " + e);

nit: I'd prefer if we'd keep "Assert." explicit, if it's an easy search and replace, otherwise nvm

@@ +52,5 @@
> +  do_print("Iterate " + dir.leafName);
> +  let entries = dir.directoryEntries;
> +
> +  // Loop over everything in this dir. If its a dir
> +  while(entries.hasMoreElements()) {

space after while

@@ +56,5 @@
> +  while(entries.hasMoreElements()) {
> +    let entry = entries.getNext();
> +    entry.QueryInterface(Ci.nsILocalFile);
> +
> +    if(entry.isDirectory()) {

space after if

@@ +73,5 @@
> +    a.QueryInterface(iid);
> +    rv = true;
> +  } catch(e) { }
> +
> +  return rv;

no need for rv, just 
try {
  QI
  return true;
} catch () {}
return false;

::: toolkit/components/feeds/test/test_xml.js
@@ +17,5 @@
>  //       - xml/ -- rss1/... 
>  //              |
>  //              -- rss2/...
>  //              |
>  //              -- atom/testcase.xml

The above comment is outdated now. no more shell.js or test.js

@@ +23,5 @@
>  
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;
> +let Cu = Components.utils;
> +let Cr = Components.results;

these are already defined in the head.js file

@@ +26,5 @@
> +let Cu = Components.utils;
> +let Cr = Components.results;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/NetUtil.jsm");

please move these to head.js

@@ +30,5 @@
> +Cu.import("resource://gre/modules/NetUtil.jsm");
> +
> +// Listens to feeds being loaded. Runs the tests built into the feed afterwards to veryify they
> +// were parsed correctly.
> +function FeedListener(testcase){

space after )

@@ +36,4 @@
>  }
>  
> +FeedListener.prototype = {
> +  handleResult: function(result){

space after )

@@ +44,2 @@
>  
> +      if(!eval(this.testcase.expect)) {

space after if

@@ +44,5 @@
>  
> +      if(!eval(this.testcase.expect)) {
> +        ok(false, "expect failed for " + this.testcase.desc);
> +      } else {
> +        ok(true, "expect passed for " + this.testcase.desc);

sounds like here you might do
ok(eval(this.testcase.expect),
   "expected value for " + this.testcase.desc);

@@ +61,3 @@
>  
> +    if (data.base == null) {
> +      uri = NetUtil.newURI('http://example.org/' + data.path, null,null);

no need for null, null

@@ +90,5 @@
> +  topDir.append("xml");
> +
> +  // Every file in the test dir contains an encapulated RSS "test". Iterate through
> +  // them all and add them to the test runner.
> +  iterateDir(topDir, true, (file) => {

no need for the round parenthesis with just one argument

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Discussed with Wes on IRC. We're going to land the patch for now and will uplift the test later once it sticks on m-c.

https://hg.mozilla.org/releases/mozilla-aurora/rev/78cc17812ef7
https://hg.mozilla.org/releases/mozilla-beta/rev/f76498a1bcbd

Comment 14

[Wesley Johnston (:wesj)]

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

Try looked good https://tbpl.mozilla.org/?tree=Try&rev=6374204a8faf
RyanVM said he'd uplift these for me.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c086369709c6

The test patch need some rebasing for beta.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fdb7508040a1

Comment 17

[custom.firefox.lady [:tawn]]

Confirmed fixed on current versions of beta, aurora, and nightly (fx 32, 33, 34). Tested on Win7.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Thanks custom.firefox.lady!

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch for beta

Not sure if we care about uplifting the tests to beta or not, but here's a patch.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to Wesley Johnston (:wesj) from comment #19)
> Not sure if we care about uplifting the tests to beta or not, but here's a
> patch.

If you uplifted the fix, you should uplift the tests.