481437 - browser_library_open_leak.js is throwing

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

see bug 481406 for more informations.

chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_open_leak.js

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_open_leak.js | [JavaScript Error: "[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIChannel.contentType]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/FeedProcessor.js :: FP_onStartRequest :: line 1440"  data: no]" {file: "file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/FeedProcessor.js" line: 1440}]

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_open_leak.js | [JavaScript Error: "[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setItemAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/nsLivemarkService.js :: LLL__setResourceTTL :: line 1068"  data: no]" {file: "file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/nsLivemarkService.js" line: 1068}]

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_open_leak.js | [JavaScript Error: "[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIChannel.contentType]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/FeedProcessor.js :: FP_onStartRequest :: line 1440"  data: no]" {file: "file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/FeedProcessor.js" line: 1440}]

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_open_leak.js | [JavaScript Error: "[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setItemAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/nsLivemarkService.js :: LLL__setResourceTTL :: line 1068"  data: no]" {file: "file:///c:/obj-i686-pc-mingw32/browser/dist/bin/components/nsLivemarkService.js" line: 1068}]

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

this in reality is an inherit from previous tests, trying to load an invalid feed throws, and also onRemoveItem we cancel the loadGroup, and then we try to set annotations on no more valid itemId...
not yet sure this is the best way to fix this though, but at least marks the points where we fail.

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 365542 [details] [diff] [review]
patch v1.0

dietrich, any thought on the approach?

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 365542 [details] [diff] [review]
patch v1.0

>diff --git a/toolkit/components/places/src/nsLivemarkService.js b/toolkit/components/places/src/nsLivemarkService.js
>--- a/toolkit/components/places/src/nsLivemarkService.js
>+++ b/toolkit/components/places/src/nsLivemarkService.js
>@@ -648,30 +648,38 @@ LivemarkLoadListener.prototype = {
>       throw Cr.NS_ERROR_UNEXPECTED;
> 
>     var channel = aRequest.QueryInterface(Ci.nsIChannel);
> 
>     // Parse feed data as it comes in
>     this._processor = Cc[FP_CONTRACTID].createInstance(Ci.nsIFeedProcessor);
>     this._processor.listener = this;
>     this._processor.parseAsync(null, channel.URI);
>-
>-    this._processor.onStartRequest(aRequest, aContext);
>+    try {
>+      this._processor.onStartRequest(aRequest, aContext);
>+    } catch (ex) {
>+      // Bad feed URI.
>+    }
>   },

1. what do you mean by "bad" here? network level error? malformed response? i assume we check for validly formed URI before this point...

2. i think you should report via Cu.reportError in the catch block, so feed failures are visible in the console.

otherwise looks fine, r=me.

Comment 4

[Marco Bonardo [:mak]]

well it is not a feed, for example in tests we use fake addresses that don't even resolve.

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> well it is not a feed, for example in tests we use fake addresses that don't
> even resolve.

ok, please put a comment there that indicates what kind of bad failure would throw.

Comment 6

[Serge Gautherie (:sgautherie)]

{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1246882480.1246889689.29547.gz&fulltext=1
Linux mozilla-central unit test on 2009/07/06 05:14:40
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1246880096.1246883979.30704.gz&fulltext=1
OS X 10.5.2 mozilla-central unit test on 2009/07/06 04:34:56
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1246882480.1246888671.17310.gz&fulltext=1
WINNT 5.2 mozilla-central unit test on 2009/07/06 05:14:40
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1246880126.1246884253.1313.gz&fulltext=1
Linux mozilla-1.9.1 unit test on 2009/07/06 04:35:26
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1246880187.1246883146.21318.gz&fulltext=1
OS X 10.5.2 mozilla-1.9.1 unit test on 2009/07/06 04:36:27
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1246880129.1246884688.5893.gz&fulltext=1
WINNT 5.2 mozilla-1.9.1 unit test on 2009/07/06 04:35:29

Running chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_open_leak.js...
Running chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_library_panel_leak.js...
}

Not throwing (anymore), but not reporting anything either :-/

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 365542 [details] [diff] [review]
patch v1.0


What do you want to do with this patch (now)?

Comment 8

[Marco Bonardo [:mak]]

the patch is still useful, those exceptions are still fired by other tests too (for example placesTxn.js)

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 10

[Marco Bonardo [:mak]]

while there, since this is still throwing in placesTxn.js i fixed another recent throw seen there.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 386990 [details] [diff] [review]
patch v1.1

r=sdwilsh

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/594b4a89b374

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> not reporting anything either :-/

While there, could you add an |ok(true, "...");| at least?

Comment 14

[Marco Bonardo [:mak]]

(In reply to comment #13)
> (In reply to comment #6)
> > not reporting anything either :-/
> 
> While there, could you add an |ok(true, "...");| at least?

done
http://hg.mozilla.org/mozilla-central/rev/b7a4a3444ba9

Comment 15

[Serge Gautherie (:sgautherie)]

Thanks.

Comment 16

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv