490028 - JavaScript strict warning in chrome://messenger-newsblog/content/Feed.js and feed-parser.js

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Karsten Düsterloh]

On starting up my SeaMonkey trunk (1.9.1) build, I get 

JavaScript strict warning: chrome://messenger-newsblog/content/Feed.js, line 24: reference to undefined property this.mFeeds[this.normalizeHost(aUrl)]

dumped to the shell thrice, although they don't appear in the Error Console...

Comment 1

[Karsten Düsterloh]

By now they do, accompanied by 

Warning: anonymous function does not always return a value
Source File: chrome://messenger-newsblog/content/Feed.js
Line: 160, Column: 16
Source Code:
  },  

and

Warning: assignment to undeclared variable url
Source File: chrome://messenger-newsblog/content/feed-parser.js
Line: 502

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #1)

Let's fix these first.

> Warning: anonymous function does not always return a value
> Source File: chrome://messenger-newsblog/content/Feed.js
> Line: 160, Column: 16
> Source Code:
>   },  

Right, blamed to sayrer:
http://hg.mozilla.org/comm-central/rev/cb36617b6a11

> Warning: assignment to undeclared variable url
> Source File: chrome://messenger-newsblog/content/feed-parser.js
> Line: 502

Right, blamed to rq:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mail/extensions/newsblog/content/feed-parser.js&mark=1.18

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> Right, blamed to sayrer:
> Right, blamed to rq:

Swap sayrer <-> rq.

Comment 4

[Rimas Kudelis]

Line 160 is not my code:

http://hg.mozilla.org/comm-central/annotate/cb36617b6a11/mailnews/extensions/newsblog/content/Feed.js#l160

or am I mistaken?

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> or am I mistaken?

Yes, you are quoting the line from the bare source file, whereas the error reports the line from the "preprocessed" file.

Comment 7

[Rimas Kudelis]

so, how can I check the preprocessed file?..

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> so, how can I check the preprocessed file?..

Any build should do.
But what is the need? The error and the code are explicit enough, no?

Comment 9

[Rimas Kudelis]

not for me, no.

Plus, looking at the commit link, I don't think i've introduced an anonymous function, did I?

Sorry, you'll have to educate me more on that if you want me to fix this. Or just do it yourself... I'm a total noob wrt mozilla inner workings.

Comment 10

[Rimas Kudelis]

well, unless you just want me to change this line:
http://hg.mozilla.org/comm-central/annotate/cb36617b6a11/mailnews/extensions/newsblog/content/Feed.js#l194

to:

return feed.parse(); // parse will asynchronously call the download callback when it is done

That, I can do :)

Comment 11

[Rimas Kudelis]

or I could replace this (line 183):
return Feed.prototype.onDownloadError(aEvent);

with this:
{
Feed.prototype.onDownloadError(aEvent);
return;
}

so that it never returns anything.

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> Plus, looking at the commit link, I don't think i've introduced an anonymous
> function, did I?

You modified
177  onDownloaded: function(aEvent)
Noone said you created it.

> Sorry, you'll have to educate me more on that if you want me to fix this.

183      return Feed.prototype.onDownloadError(aEvent);
(now) returns a value,

but
194 feed.parse(); // parse will asynchronously call the download callback when it is done
195 },
(still) does not

> Or just do it yourself... I'm a total noob wrt mozilla inner workings.

I have no idea what this function should do: let's reopen bug 491720!
It's not (only) "mozilla", it's _javascript_ in the first place.

Comment 13

[Rimas Kudelis]

(In reply to comment #12)
> (In reply to comment #9)
> > Plus, looking at the commit link, I don't think i've introduced an anonymous
> > function, did I?
> 
> You modified
> 177  onDownloaded: function(aEvent)
> Noone said you created it.
> 
> > Sorry, you'll have to educate me more on that if you want me to fix this.
> 
> 183      return Feed.prototype.onDownloadError(aEvent);
> (now) returns a value,
> 
> but
> 194 feed.parse(); // parse will asynchronously call the download callback when
> it is done
> 195 },
> (still) does not
> 
> > Or just do it yourself... I'm a total noob wrt mozilla inner workings.
> 
> I have no idea what this function should do: let's reopen bug 491720!
> It's not (only) "mozilla", it's _javascript_ in the first place.

yeah, righ.

The secret is that Feed.prototype.onDownloadError(aEvent) never returns anything, so basically, if I get it right, returning its result is the same as returning nothing. Thus technically, this function never returns a value.


CC'ing Myk. Myk, would you help me with this? Is solution in Comment #11 the right thing to do?

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)

> The secret is that Feed.prototype.onDownloadError(aEvent) never returns
> anything,

As you can see, neither the JSEngine nor me did guess that.

> returning its result is the same as returning nothing.

Except that asking to return a non-existent value does not make sense in the first place.

> Is solution in Comment #11 the right thing to do?

It obviously is ;-)

Comment 15

[Rimas Kudelis]

OK, so I can provide a patch next week, get a review etc. But if someone else does (blink blink), I'd be grateful. :)

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #14)
> (In reply to comment #13)
> Except that asking to return a non-existent value does not make sense in the
> first place.
> 
> > Is solution in Comment #11 the right thing to do?
> 
> It obviously is ;-)

Yup, I concur, this is the right solution.

Comment 17

[Rimas Kudelis]

Attachment: [committed] Feed.js patch

Thanks for waiting, here's the patch I promised.

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 380936 [details] [diff] [review]
[committed] Feed.js patch

>     if (request.status < 200 || request.status >= 300)
>+	 {

Nits: join the two lines.

Comment 19

[Rimas Kudelis]

Bah, blame vim. Fixed locally, waiting for r+ to commit.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 380936 [details] [diff] [review]
[committed] Feed.js patch

Looks good! r=myk with Serge's nit fixed.

Comment 21

[David :Bienvenu]

Comment on attachment 380936 [details] [diff] [review]
[committed] Feed.js patch

thx!

Comment 22

[Rimas Kudelis]

http://hg.mozilla.org/comm-central/rev/72082583daf3

Not marking fixed since feed-parser.js is still there, right?

Comment 23

[Rimas Kudelis]

Attachment: feed-parser.js patch

I guess this is what feed-parser.js patch should look like, right?

Comment 24

[David :Bienvenu]

Comment on attachment 381153 [details] [diff] [review]
feed-parser.js patch

you can just return 

ioService.newURI(alink.baseURI, null, null).resolve(alink.getAttribute('href'));

Comment 25

[Rimas Kudelis]

Attachment: [committed] feed-parser.js patch, v.1.1

Comment 26

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 381333 [details] [diff] [review]
[committed] feed-parser.js patch, v.1.1

Great! r=myk

Comment 27

[Rimas Kudelis]

Comment on attachment 381333 [details] [diff] [review]
[committed] feed-parser.js patch, v.1.1

OK, since it's bienvenu's suggestion, I took liberty to commit it without his superreview:
http://hg.mozilla.org/comm-central/rev/7f60d259141f

Comment 28

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> JavaScript strict warning: chrome://messenger-newsblog/content/Feed.js, line
> 24: reference to undefined property this.mFeeds[this.normalizeHost(aUrl)]

Karsten, are you still seeing this (initial) warning?

Comment 29

[Karsten Düsterloh]

(In reply to comment #28)
> (In reply to comment #0)
> > JavaScript strict warning: chrome://messenger-newsblog/content/Feed.js, line
> > 24: reference to undefined property this.mFeeds[this.normalizeHost(aUrl)]
> 
> Karsten, are you still seeing this (initial) warning?

Yes, thrice, in the very same line.

Comment 30

[Serge Gautherie (:sgautherie)]

Code is
{
57   getFeed: function (aUrl)
58   {
59     return this.mFeeds[this.normalizeHost(aUrl)];
60   },
}

Comment 31

[Karsten Düsterloh]

Attachment: fix the real cause [pushed: comment #33]

The basic problem here is the implementation of FeedCache: mFeeds is initialized as of type Array, but used as an Object. The first time a feed is sought in the cache, everything is fine - the cache is empty and of type Array, the given URL is not a valid index, no problem. Then mFeeds is treated as (and internally turned into) an Object when adding the feed using the normalized URL as an index. Any subsequent call to getFeed with a yet unknown feed URL will result in a strict warning...

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 381955 [details] [diff] [review]
fix the real cause [pushed: comment #33]

I haven't built and tested SeaMonkey with this patch, but based on visual inspection and grepping the code for references to mFeeds, this is obviously correct. r=myk

Note that progressNotifier::mFeeds in newsblog.js has the same problem of being initialized to an Array while being used as an object.

Comment 33

[Karsten Düsterloh]

Pushed as <http://hg.mozilla.org/comm-central/rev/6062bc855a1c>.

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Fix newsblog.js too [Checkin: Comment 36]

(In reply to comment #32)
> Note that progressNotifier::mFeeds in newsblog.js has the same problem of being
> initialized to an Array while being used as an object.

http://mxr.mozilla.org/comm-central/search?string=mFeeds&case=on&find=%2Fnewsblog%5C.js%24

Untested, but seems trivial.

Comment 35

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 382588 [details] [diff] [review]
(Dv1) Fix newsblog.js too
[Checkin: Comment 36]

This looks great and seems to behave the same in my tests. r=myk

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 382588 [details] [diff] [review]
(Dv1) Fix newsblog.js too
[Checkin: Comment 36]


http://hg.mozilla.org/comm-central/rev/4ee756629083