481227 - nsAnnoProtocolHandler gets favicon data synchronously from the database

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Shawn Wilsher :sdwilsh]

We grab the favicon data from the db synchronously in our annotation protocol handler.  What we really want to do is grab this asynchronously, and channels let us do this.

This makes me sad :(

Comment 1

[Shawn Wilsher :sdwilsh]

Basically, what we want to do here is use NS_NewPipe, and pass the output stream to our async callback to the db.  I think for segment size, we want to pass MAX_FAVICON_SIZE, since anything larger we will optimize to a smaller size.  When the db gets data, it will write that data to the output stream.  When we want push that data to the output stream, we should call write.  I'm not sure we should do when the return value is less than aCount though...

At least, I think that's what we want to do.  Need to verify that.

Comment 2

[Shawn Wilsher :sdwilsh]

OK, write shouldn't ever fail for us unless we run out of memory.

If the return value of write is less than aCount, we can retry (assuming it's not an error)

Comment 3

[Shawn Wilsher :sdwilsh]

This might impact location bar searches too for large databases.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v0.1

This works when we actually have the data.  Doesn't work if we don't have a favicon yet, as I need to figure out how to redirect the channel.

This also doesn't have any tests to make sure this is right.  I'm not sure if we have tests for this stuff, so I'll be adding them if we don't.

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This doesn't have any tests (and we have no existing ones, sadly.  I've played with it in real builds though, and it works correctly.

I'm putting this up for review, and I'll post some tests in a bit once I finish them.

Comment 6

[Shawn Wilsher :sdwilsh]

Try server builds with code almost identical to attachment 365529 [details] [diff] [review] (I did some refactoring) are here:
https://build.mozilla.org/tryserver-builds/2009-03-04_13:[e-mail]-try-9bb80e57a92/

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: test v1.0

Sadly, this test triggers the following leak:
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 12 bytes during test execution
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsStringBuffer with size 8 bytes
TEST-UNEXPECTED-FAIL | runtests-leaks | leaked 1 instance of nsTArray_base with size 4 bytes

It goes away if I comment out:
  fs.setFaviconDataFromDataURL(uri(testURIs[1]), expectedURIs[1],
                               (Date.now() + 60 * 60 * 24 * 1000) * 1000);

I'll file a new bug on that once I figure out what the cause is.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: test v1.1

helps if I qrefresh first...

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 366404 [details] [diff] [review]
test v1.1

Strike two - not even this bugs patch...

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 365529 [details] [diff] [review]
v1.0

>diff --git a/toolkit/components/places/src/nsFaviconService.cpp b/toolkit/components/places/src/nsFaviconService.cpp
>+nsresult
>+nsFaviconService::GetFaviconDataAsync(nsIURI* aFaviconURI,
>+                                      mozIStorageStatementCallback *aCallback)
>+{
>+  NS_ASSERTION(aCallback, "Doesn't make sense to call this without a callback");
>+  nsresult rv = BindStatementURI(mDBGetData, 0, aFaviconURI);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<mozIStoragePendingStatement> pendingStatement;
>+  return mDBGetData->ExecuteAsync(aCallback, getter_AddRefs(pendingStatement));
>+}

If the only scope of this method is to expose the cached mDBGetData statement, is there a reason to not instead simply make nsAnnoProtocolHandler a friend class of nsFaviconService? This is a public method, do you think to expose this in idl in future?

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> If the only scope of this method is to expose the cached mDBGetData statement,
> is there a reason to not instead simply make nsAnnoProtocolHandler a friend
> class of nsFaviconService? This is a public method, do you think to expose this
> in idl in future?
Because exposing member variables via "friend" is really hacky, whereas adding a new function, even if it's only used internally to places, is not exposing internal state of nsFaviconService.  If we ever have to add invariants on the statement, we only have to worry about nsFaviconService instead of some arbitrary number of "friend" classes.

Is there any reason you canceled my review request to you?

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 365529 [details] [diff] [review]
v1.0

Friends classes have been created for that purpose, exposing private stuff to a known not-evil class, while a public method is "usually" part of an interface.
Btw, why not simply exposing a method that will return the statement? i'm simply guessing if you're thinking of future uses for that method.

>diff --git a/toolkit/components/places/src/nsAnnoProtocolHandler.cpp b/toolkit/components/places/src/nsAnnoProtocolHandler.cpp
>+static
>+nsresult
>+GetDefaultIcon(nsIChannel **aChannel)
>+{
>+  nsresult rv;
>+  nsCOMPtr<nsIURI> uri;

make this less generic, as defaultIconURI

>+
>+  NS_IMETHOD HandleResult(mozIStorageResultSet *aResultSet)
>+  {
>+    // We will only get one row back in total, so we do not need to loop.
>+    nsCOMPtr<mozIStorageRow> row;
>+    nsresult rv = aResultSet->GetNextRow(getter_AddRefs(row));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // We do not allow favicons without a MIME type, so we'll return the default
>+    // icon.

specify that is done through early return with mReturnDefaultIcon true

>+
>+  NS_IMETHOD HandleCompletion(PRUint16 aReason)
>+  {
>+    if (!mReturnDefaultIcon)
>+      return mOutputStream->Close();

Add a comment, this happens if we did not encounter any problem while retrieving the icon
If this fails, would not be better trying to return the default icon still?

>-  // get the data from the annotation service and hand it off to the stream
>+  // special handling for favicons: ask favicon service

this comment is a bit cryptic

>+nsresult
>+nsAnnoProtocolHandler::NewFaviconChannel(nsIURI *aURI, nsIURI *aAnnotationURI,
>+                                         nsIChannel **_channel)
>+{

is aURI really aFaviconURI?

>+  // Create our pipe.  This will give us our input stream and output stream
>+  // that will be written to when we get data from the database.
>+  nsresult rv;

define rv on first use below

>diff --git a/toolkit/components/places/src/nsFaviconService.h b/toolkit/components/places/src/nsFaviconService.h
>--- a/toolkit/components/places/src/nsFaviconService.h
>+++ b/toolkit/components/places/src/nsFaviconService.h
>@@ -41,18 +41,24 @@
> #include "nsIFaviconService.h"
> #include "nsServiceManagerUtils.h"
> #include "nsString.h"
> #include "mozIStorageConnection.h"
> #include "mozIStorageValueArray.h"
> #include "mozIStorageStatement.h"
> #include "nsIObserver.h"
> 
>+// Favicons bigger than this size should not be saved to the db to avoid
>+// bloating it with large image blobs.
>+// This still allows us to accept a favicon even if we cannot optimize it.
>+#define MAX_FAVICON_SIZE 10240
>+
> // forward definition for friend class
> class FaviconLoadListener;
>+class mozIStorageStatementCallback;

add a newline, or sounds like both are friend classes. Would not be enough including mozIStorageStatementCallback.h ?

Comment 13

[Shawn Wilsher :sdwilsh]

Note: with the patch in bug 482614, we no longer leak on this test.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Friends classes have been created for that purpose, exposing private stuff to a
> known not-evil class, while a public method is "usually" part of an interface.
> Btw, why not simply exposing a method that will return the statement? i'm
> simply guessing if you're thinking of future uses for that method.
Friend classes expose all or nothing, so it becomes easy to abuse that.  And your code suddenly becomes much more dependent on another class (and in our case, it is only loosely related).
Exposing just the statement means that the annotation protocol has to know the right order to bind the parameters (and what parameters for that matter).  I'm already not happy with the fact that we have to know what columns to get back, but it's at least documented in the javadoc comments for the method.
We might decide that this method would be good to stick on nsIFaviconService, but I thought we'd be better off making sure it's a sensible API by using it internally first.

> specify that is done through early return with mReturnDefaultIcon true
This is already specified in the comment above the class (it describes how it works).  I've tweaked the wording a bit to be more clear on this, but I don't think it's wise to have a comment every time we return early about why it's OK.

> Add a comment, this happens if we did not encounter any problem while
> retrieving the icon
Same as above

> If this fails, would not be better trying to return the default icon still?
No - if we can't close our output stream we are pretty much screwed.  We'd have to close it to finish sending the default icon anyway.

> >+  // special handling for favicons: ask favicon service
> 
> this comment is a bit cryptic
This was just a code move from before (just a whitespace change), but I'll update it.

> is aURI really aFaviconURI?
No, it's the original URI that the request was made for.  I've added some comments to better explain that in the .h file

> add a newline, or sounds like both are friend classes. Would not be enough
> including mozIStorageStatementCallback.h ?
It would, but it means longer compile times for any file that includes this header file but doesn't need the full definition of mozIStorageStatementCallback.  It's better to do forward declarations in header files whenever possible, IMO.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses review comments

Comment 16

[Shawn Wilsher :sdwilsh]

Attachment: test v2.0

This test passes before and after my patch.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 366719 [details] [diff] [review]
v1.1

>+++ b/toolkit/components/places/src/nsAnnoProtocolHandler.cpp
>+GetDefaultIcon(nsIChannel **aChannel)

Maybe we should consider adding an nsNetUtil function to call IOService's newChannel().  In any case, this is fine for now.

> + * However, if an error occurs at any point, we do not set mReturnDefaultIcon to
> + * true,

You mean to false?

>+class faviconAsyncLoader : public mozIStorageStatementCallback

mozFaviconAsyncLoader or something?

>+    , mReturnDefaultIcon(true)

Why bool and not PRBool?

>+  NS_IMETHOD HandleResult(mozIStorageResultSet *aResultSet)
>+    PRUint32 bytesWritten = 0;
>+    do {
>+      // We can keep trying as long as we have a success code and we haven't
>+      // written all of our bytes.
>+      rv = mOutputStream->Write(reinterpret_cast<const char *>(favicon), size,
>+                                &bytesWritten);
>+    } while (NS_SUCCEEDED(rv) && bytesWritten < size);

No, this is wrong.  Say |size| is 10 and the first write writes 5 bytes. bytesWritten will be 5.  Then you'll loop for a second try and pass in the original data again.  This time say bytesWritten is 3.  You'll loop again, etc.  The loop might not even terminate.  The right way to do this is:

  PRUint32 totalWritten = 0;
  do {
    PRUint32 bytesWritten;
    rv = mOutputStream->Write(reinterpret_cast<const char*>(favicon) +
                                totalWritten,
                              size - totalWritten, &bytesWritten);
    if (NS_FAILED(rv) || !bytesWritten) {
      break;
    }
    totalWritten += bytesWritten;
  } while (1);
  NS_ASSERTION(NS_FAILED(rv) || totalWritten == size,
               "Failed to write all data");

or some such.

>+  NS_IMETHOD HandleError(mozIStorageError *aError)

Care to wrap all that debug code in #ifdef DEBUG?  (Everything except the return statement.)

>+  NS_IMETHOD OnStopRequest(nsIRequest *, nsISupports *, nsresult aStatusCode)
>+    // But, we'll warn about it not being successful if it wasn't.
>+    if (NS_FAILED(aStatusCode))
>+      NS_WARNING("Got an error when trying to load our default favicon!");

  NS_WARN_IF_FALSE(NS_SUCCEEDED(aStatusCode), ....)

>+private:
>+  faviconAsyncLoader() {}

Do you expect anyone to use it?  If not, just leave it unimplemented, so attempts will not compile?

The rest looks good.

Comments on the test:

1) s/exptected/expected/
2) Can add a test that the favicon channel is returning the right MIME type?

r+sr=bzbarsky with the Write() business fixed.

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 366720 [details] [diff] [review]
test v2.0

I hope tinderboxes will like this, so it's worth trying to push it and see how they react.

>diff --git a/toolkit/components/places/tests/chrome/test_favicon_annotations.xul b/toolkit/components/places/tests/chrome/test_favicon_annotations.xul

>+/**
>+ * This file tests the moz-anno protocol and how it loads favicons.
>+ */
>+-->

reference bug number please, i know we can find it through blame, but is still faster having an help about where to start :)

>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cr = Components.results;

a newline here helps readability

>+let fs = Cc["@mozilla.org/browser/favicon-service;1"].
>+         getService(Ci.nsIFaviconService);
>+
>+// Test descriptions that will be printed in the case of failure.
>+let testDescriptions = [
>+  "moz-anno URI with not data in the database loads default icon",

nit: "with not" == without (or "with no data")?

>+// URIs to load for exptected results.

nit: expected

>+
>+/**
>+ * The event listener placed on our test windows used to determine when it is
>+ * safe to compare the two windows.
>+ */
>+let _results = [];
>+function loadEventHandler()
>+{
>+  _results.push(snapshotWindow(window));
>+
>+  loadNextTest();
>+}

just to be sure, snapshotWindow is sync, or do we have any risk of timeouts on slow tinderboxes?

>+  let nextURI = function() {
>+    let index = Math.floor(_counter / 2);
>+    if ((_counter % 2) == 0)
>+      return "moz-anno:favicon:" + testURIs[index];

GetFaviconLinkForIcon would probably do this for you without the need to hardcode moz-anno: and favicon: in the test

>+
>+function test()
>+{
>+  let db = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsPIPlacesDatabase).
>+           DBConnection;
>+
>+  // Empty any old downloads

Uh? did you mean favicons?

>+    <p id="display"></p>
>+    <div id="content" style="display:none;"></div>
>+    <pre id="test"></pre>

what are these fields for?

Comment 19

[Boris Zbarsky [:bzbarsky]]

snapshotWindow is sync, yes.

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> Why bool and not PRBool?
Because it's a variable that never has to go through xpconnect, so I am going to take advantage of the compiler checking types for me.  I've asked in #developers before, and nobody said it would be a bad thing if it's used in situations that do not go through xpconnect.  I didn't change this in the patch, so please comment if you still want me to change it.

> No, this is wrong.  Say |size| is 10 and the first write writes 5 bytes.
> bytesWritten will be 5.  Then you'll loop for a second try and pass in the
> original data again.  This time say bytesWritten is 3.  You'll loop again, etc.
>  The loop might not even terminate.  The right way to do this is:
Errr, yeah.  Not sure what I was thinking there.  Fixed per discussion on irc.


>   NS_WARN_IF_FALSE(NS_SUCCEEDED(aStatusCode), ....)
neat - didn't know about that!

> >+private:
> >+  faviconAsyncLoader() {}
> 
> Do you expect anyone to use it?  If not, just leave it unimplemented, so
> attempts will not compile?
Interesting.  I thought the compiler provided one for free, so you had to mark it private if you didn't want it to work, but a test shows otherwise.  Thanks for pointing that out!

> 2) Can add a test that the favicon channel is returning the right MIME type?
I wasn't really sure how to go about that.  If you point me in the right direction, I'll be happy to go ahead and write a test for that.

(In reply to comment #18)
> I hope tinderboxes will like this, so it's worth trying to push it and see how
> they react.
Try server, which admittedly only does linux tests right now, was OK with it.

> reference bug number please, i know we can find it through blame, but is still
> faster having an help about where to start :)
Sure, but bug 316077 doesn't really give any useful context AFAICT

> >+// URIs to load for exptected results.
> 
> nit: expected
I see Komodo doesn't do spell checking automatically like vim does :(

> GetFaviconLinkForIcon would probably do this for you without the need to
> hardcode moz-anno: and favicon: in the test
I was going to use that, but the idl doesn't guarantee that we will get an annotation URI, and since that's what we want to test, I want to make sure it's what we get.

> what are these fields for?
Test boilerplate - it's where the results get printed.

Comment 21

[Shawn Wilsher :sdwilsh]

Attachment: test v2.1

Addresses test review comments

Comment 22

[Shawn Wilsher :sdwilsh]

Pushed the test:
http://hg.mozilla.org/mozilla-central/rev/fd60cf7b06a7

Comment 23

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Addresses review comments.

Comment 24

[Shawn Wilsher :sdwilsh]

Pushed the test to 1.9.1 now too:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9b8d00e46260

Comment 25

[Shawn Wilsher :sdwilsh]

I think I have a unit test to check the content type is being set.  Having issues getting any unit tests to run though, so I'll hold off on posting it.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> > 2) Can add a test that the favicon channel is returning the right MIME type?
> I wasn't really sure how to go about that. 

If you have a point in the test at which loading a given anno: URI will give back a favicon, you could just manually do the load and then see what it returns...

In the patch, you want size != totalWritten as the loop termination condition, no?

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> If you have a point in the test at which loading a given anno: URI will give
> back a favicon, you could just manually do the load and then see what it
> returns...
In the xpcshell test I'm writing, I'm opening a new channel and checking the content type in onDataAvailable.

> In the patch, you want size != totalWritten as the loop termination condition,
> no?
Well, I want that to be my loop continuation condition, which means the termination condition should be size == totalWritten.  I've got it backwards in the patch, so good catch.

Comment 28

[Shawn Wilsher :sdwilsh]

Attachment: content-type test v1.0

xpcshell test for checking that the content type gets set

Comment 29

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

For checkin.  Addresses all comments and passes all tests.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 366953 [details] [diff] [review]
content-type test v1.0

Looks ok, with one change: the channel.contentType check needs to be in onStartRequest.  That's when the type needs to be correct; any time after that is too late.

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #30)
> (From update of attachment 366953 [details] [diff] [review])
> Looks ok, with one change: the channel.contentType check needs to be in
> onStartRequest.  That's when the type needs to be correct; any time after that
> is too late.
Is it problematic that we don't set that type until after we get data from the database?  I'm not 100% sure when onStartRequest would be sent, but it if's right after asyncOpen is called, but before handleResult is called for our loader, we won't have set the right type.

Comment 32

[Shawn Wilsher :sdwilsh]

(In reply to comment #31)
> Is it problematic that we don't set that type until after we get data from the
> database?  I'm not 100% sure when onStartRequest would be sent, but it if's
> right after asyncOpen is called, but before handleResult is called for our
> loader, we won't have set the right type.
Guess not - test passes with and without my changes, so looks good.

Comment 33

[Shawn Wilsher :sdwilsh]

Pushed the xpcshell test case:
http://hg.mozilla.org/mozilla-central/rev/bf3a15860df4

Comment 34

[Shawn Wilsher :sdwilsh]

xpcshell test case to 1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0847b6318522

Comment 35

[Shawn Wilsher :sdwilsh]

and the code changes to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/04abfb92ff2a

Comment 36

[Shawn Wilsher :sdwilsh]

Comment on attachment 366954 [details] [diff] [review]
v1.3

Asking for approval for 1.9.1.  This is a low risk patch that has tests for the behaviors it has changed.  It results in less IO on the main thread from disk.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 366954 [details] [diff] [review]
v1.3

a191=beltzner

Comment 38

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/48efe3640a2a