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

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: tawn, Assigned: wesj)

Tracking

({branch-patch-needed, regression})

Trunk
mozilla34
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32+ verified, firefox33+ verified, firefox34 verified)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
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
Status: UNCONFIRMED → NEW
QA Whiteboard: [bugday-20140804]
Ever confirmed: true
Flags: needinfo?(wjohnston)
Keywords: regression
OS: Windows 7 → All
Posted patch PatchSplinter Review
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.
Attachment #8467207 - Flags: review?(mak77)
Flags: needinfo?(wjohnston)
Assignee: nobody → wjohnston
Blocks: 998063
Component: Untriaged → General
Product: Firefox → Toolkit
Posted patch Tests Patch (obsolete) — Splinter Review
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
Attachment #8467312 - Flags: review?(mak77)
Flags: needinfo?(philringnalda)
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?"
Flags: needinfo?(philringnalda)
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 :)
Attachment #8467312 - Flags: review?(mak77) → feedback+
Attachment #8467207 - Flags: review?(mak77) → review+
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.
Attachment #8467207 - Flags: approval-mozilla-beta?
Attachment #8467207 - Flags: approval-mozilla-aurora?
are you going to manage the tests elsewhere? otherwise this is a leave-open
Keywords: leave-open
Duplicate of this bug: 1048140
Attachment #8467207 - Flags: approval-mozilla-beta?
Attachment #8467207 - Flags: approval-mozilla-beta+
Attachment #8467207 - Flags: approval-mozilla-aurora?
Attachment #8467207 - Flags: approval-mozilla-aurora+
Posted patch TestsSplinter Review
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
Attachment #8467312 - Attachment is obsolete: true
Attachment #8470327 - Flags: review?(mak77)
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
Attachment #8470327 - Flags: review?(mak77) → review+
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
Flags: in-testsuite?
Keywords: leave-open
Keywords: checkin-needed
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/releases/mozilla-aurora/rev/c086369709c6

The test patch need some rebasing for beta.
Flags: needinfo?(wjohnston)
Flags: in-testsuite?
Flags: in-testsuite+
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
https://hg.mozilla.org/mozilla-central/rev/fdb7508040a1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Confirmed fixed on current versions of beta, aurora, and nightly (fx 32, 33, 34). Tested on Win7.
Thanks custom.firefox.lady!
Not sure if we care about uplifting the tests to beta or not, but here's a patch.
Flags: needinfo?(wjohnston)
(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.
Duplicate of this bug: 1046939
You need to log in before you can comment on or make changes to this bug.