Closed Bug 490028 Opened 13 years ago Closed 13 years ago

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

Categories

(MailNews Core :: Feed Reader, defect)

All
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: mnyromyr, Assigned: mnyromyr)

References

()

Details

Attachments

(4 files, 1 obsolete file)

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...
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
Summary: JavaScript strict warning in chrome://messenger-newsblog/content/Feed.js → JavaScript strict warning in chrome://messenger-newsblog/content/Feed.js and feed-parser.js
(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
Blocks: 491720
(In reply to comment #2)
> Right, blamed to sayrer:
> Right, blamed to rq:

Swap sayrer <-> rq.
(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.
Duplicate of this bug: 490029
so, how can I check the preprocessed file?..
(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?
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.
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 :)
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.
(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.
No longer blocks: 491720
Depends on: 491720
(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?
(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 ;-)
OK, so I can provide a patch next week, get a review etc. But if someone else does (blink blink), I'd be grateful. :)
Component: Backend → Feed Reader
QA Contact: backend → feed.reader
Hardware: x86 → All
Blocks: 491720
No longer depends on: 491720
(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.
Thanks for waiting, here's the patch I promised.
Attachment #380936 - Flags: review?(myk)
Comment on attachment 380936 [details] [diff] [review]
[committed] Feed.js patch

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

Nits: join the two lines.
Bah, blame vim. Fixed locally, waiting for r+ to commit.
Comment on attachment 380936 [details] [diff] [review]
[committed] Feed.js patch

Looks good! r=myk with Serge's nit fixed.
Attachment #380936 - Flags: superreview?(bienvenu)
Attachment #380936 - Flags: review?(myk)
Attachment #380936 - Flags: review+
Attachment #380936 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 380936 [details] [diff] [review]
[committed] Feed.js patch

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

Not marking fixed since feed-parser.js is still there, right?
Whiteboard: Feed.js fixed
Attached patch feed-parser.js patch (obsolete) — Splinter Review
I guess this is what feed-parser.js patch should look like, right?
Attachment #381153 - Flags: superreview?
Attachment #381153 - Flags: review?
Attachment #380936 - Attachment description: the patch → [committed] Feed.js patch
Attachment #381153 - Flags: superreview?(bienvenu)
Attachment #381153 - Flags: superreview?
Attachment #381153 - Flags: review?(myk)
Attachment #381153 - Flags: review?
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'));
Attachment #381153 - Attachment is obsolete: true
Attachment #381333 - Flags: superreview?(bienvenu)
Attachment #381333 - Flags: review?(myk)
Attachment #381153 - Flags: superreview?(bienvenu)
Attachment #381153 - Flags: review?(myk)
Attachment #381333 - Flags: review?(myk) → review+
Comment on attachment 381333 [details] [diff] [review]
[committed] feed-parser.js patch, v.1.1

Great! r=myk
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
Attachment #381333 - Attachment description: feed-parser.js patch, v.1.1 → [committed] feed-parser.js patch, v.1.1
Attachment #381333 - Flags: superreview?(bienvenu)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: Feed.js fixed
(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?
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Code is
{
57   getFeed: function (aUrl)
58   {
59     return this.mFeeds[this.normalizeHost(aUrl)];
60   },
}
Status: REOPENED → NEW
Whiteboard: [ToDo: comment 0+30]
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...
Assignee: nobody → mnyromyr
Attachment #381955 - Flags: review?
Attachment #381955 - Flags: review? → review?(myk)
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.
Attachment #381955 - Flags: review?(myk) → review+
Pushed as <http://hg.mozilla.org/comm-central/rev/6062bc855a1c>.
Status: NEW → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Attachment #381955 - Attachment description: fix the real cause → fix the real cause [pushed: comment #33]
Whiteboard: [ToDo: comment 0+30]
(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.
Attachment #382588 - Flags: review?(myk)
Attachment #382588 - Flags: superreview?(bienvenu)
Attachment #382588 - Flags: review?(myk)
Attachment #382588 - Flags: review+
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
Attachment #382588 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 382588 [details] [diff] [review]
(Dv1) Fix newsblog.js too
[Checkin: Comment 36]


http://hg.mozilla.org/comm-central/rev/4ee756629083
Attachment #382588 - Attachment description: (Dv1) Fix newsblog.js too → (Dv1) Fix newsblog.js too [Checkin: Comment 36]
Target Milestone: --- → mozilla1.9.1
Depends on: 498097
You need to log in before you can comment on or make changes to this bug.