404634 - http urls don't render in TestGtkEmbed

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect, P3)

Description

[Brad Lassey [:blassey] (use needinfo?)]

When you launch TestGtkEmbed built of off of the trunk, you get a black content area.  Going to any http or https url doesn't change this.  You can however navigate to file://, ftp://, about: or gopher:// urls all work.  


Once you render something that works, trying to navigate to an http or https url doesn't change the content area.

loading file:/// has the following terminal output:

loading url file:///
open_uri_cb file:///
load_started_cb
net_state_change_cb 983041
js_status_cb
js_status_cb
location_changed_cb
net_state_change_cb 196612
progress_change_cb cur 1532 max 0
net_state_change_cb 65552
title_changed_cb
net_state_change_cb 131088
net_state_change_cb 786448
load_finished_cb



loading http://mozilla.org has the following terminal output:

loading url http://mozilla.org
open_uri_cb http://mozilla.org/
load_started_cb
load_finished_cb

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can you possibly find a several-day regression range for this?  I suspect this should be pretty easy to fix once we know what changes might have caused it...

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

I've narrowed it down to between August 30 14:51:45 PDT 2007 and Sept 1 14:51:45 PDT 2007.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Looks like a regression from bug 384941.  In nsDocShell::CheckClassifier, Run() is returning an error result that's not NS_ERROR_FACTORY_NOT_REGISTERED.  In fact, it's returning NS_ERROR_OUT_OF_MEMORY.

This happens because nsUrlClassifierDBServiceWorker::Init does:

606   nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_LOCAL_50_DIR,
607                                        getter_AddRefs(mDBFile));

Which returns NS_ERROR_FAILURE, of course, since there is no profile here.  Then nsUrlClassifierDBService::GetInstance returns null (which is what it does on any init error), which the caller treats as an out-of-memory error.  The result is that you can't load anything that we'd run through the URL classifier.

We probably need to fix that...

Comment 4

[Alexander Sack]

Attachment: use a temp file if no profile exists

create a temp file with 0600 permissions if no profile path exists and remove it when shutting down cleanly.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I can't review this code, actually.  I'm not a toolkit peer.

Comment 6

[Dave Camp (:dcamp)]

I think it's pretty reasonable to fail if it can't create the database in the profile.  The database is built and maintained over time, a temporary one isn't a whole lot of use.

Another option would be for the nsUrlClassifierDBService to return successfully if creating the worker fails, and noop its methods if there's no worker.

Or it might be reasonable for the docshell to ignore the classifier for any initialization failure, not just NS_ERROR_FACTORY_NOT_REGISTERED.

(that said, I don't think the temporary db is actively harmful, just mostly wasteful)

Comment 7

[Alexander Sack]

yes its a compromise between: disabling completely or "just" session data. I understood that url-classifier would (in general) work with this "just" session approach, though there probably is a performance impact because the database always needs to be populated (thus the NS_WARNING output).

I thought about disabling the classifier also on OUT_OF_MEMORY, but found it pretty dirty ;) ... will attach the patch for that as well ...

Comment 8

[Alexander Sack]

Attachment: ignore classifier on (pseudo-)OOM

Comment 9

[Boris Zbarsky [:bzbarsky]]

If we have a legitimate OOM we want to stop loading the page (since we can't tell whether it's an attack).

Comment 10

[Alexander Sack]

Comment on attachment 290308 [details] [diff] [review]
ignore classifier on (pseudo-)OOM

agreed. we should either use the temporary file or consider the noop solution from comment #6

any idea on the impact of the temporary solution?

Comment 11

[Dave Camp (:dcamp)]

(In reply to comment #10)
> any idea on the impact of the temporary solution?
> 

By default the classifier won't populate itself, that's driven by the safebrowsing component in browser/.  So for something like TestGtkEmbed, you at least won't spend time populating the database for no good reason, you'll just be querying an empty one for no good reason :)

I don't know how common profile-less operation is, so I don't know if that's a big deal.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

I'm not sure TestGtkEmbed is even intended to be running without a profile since the code sets the profile path here:
http://lxr.mozilla.org/seamonkey/source/embedding/browser/gtk/tests/TestGtkEmbed.cpp#232

the question then is why is this path not provided by the directory service 

Comment 13

[Alexander Sack]

(In reply to comment #12)
> the question then is why is this path not provided by the directory service 
> 

Good point ... I ran into this with a python test script that didn't set the profile_path.

For TestGtkEmbed.cpp it doesn't work because the GTKEmbedDirectoryProvider only matches NS_APP_USER_PROFILE_50_DIR, but not NS_APP_USER_PROFILE_LOCAL_50_DIR.

Look: http://lxr.mozilla.org/seamonkey/source/embedding/browser/gtk/src/EmbedPrivate.cpp#228

Comment 14

[Alexander Sack]

Attachment: provide NS_APP_USER_PROFILE_LOCAL_50_DIR in GTKEmbedDirectoryProvider

Comment 15

[Christian Persch (GNOME) (away; not receiving bug mail)]

As far as I understand, this won't just affect TestGtkEmbed, but any embedding app using XRE_InitEmbedding but not setting a profile?

I think running without a profile should be supported. There have been other bugs (e.g. PSM bug 309877) where running without profile broke gecko, and they've been fixed to work without a profile.

Comment 16

[Benjamin Smedberg]

There are two bugs here: we should continue to support running without a profile. We also should not rely on a PROFILE_LOCAL key being set... gecko clients who ask for the PROFILE_LOCAL key should be prepared for the request to fail and fall back on the PROFILE key.

Comment 17

[Alexander Sack]

(In reply to comment #16)
> There are two bugs here: we should continue to support running without a
> profile. We also should not rely on a PROFILE_LOCAL key being set... gecko
> clients who ask for the PROFILE_LOCAL key should be prepared for the request to
> fail and fall back on the PROFILE key.
> 

OK, i can do the fallback thing for url-classifier then.

What about the "no profile" part? Is the temporary file approach above good enough?

Comment 18

[Benjamin Smedberg]

I don't know... I tend to think that the url-classifier just shouldn't work without a profile, and that docshell should be prepared for the url-classifier to fail.

Comment 19

[Boris Zbarsky [:bzbarsky]]

If url-classifier fails in a situation where we expect it to work docshell should fail out of the load.  See comment 9.

The problem from the docshell's point of view is that trying to use the classifier results in one of three return values: NS_OK (all good), NS_FACTORY_NOT_REGISTERED (there is no classifier), and NS_ERROR_OUT_OF_MEMORY (everything else).  That's not fine-grained enough for docshell to really make use of.

Comment 20

[Benjamin Smedberg]

But a separate I_FAILED_SERENELY_PLEASE_KEEP_GOING result would be acceptable, right?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Yes, absolutely.

Comment 22

[Alexander Sack]

Attachment: updated

add more fine grained error handling/propagation and make the classifier DB service try NS_APP_USER_PROFILE_50_DIR if NS_APP_USER_PROFILE_LOCAL_50_DIR is not available.

is NS_ERROR_NOT_AVAILABLE ok, or do we need to introduce a I_FAILED_SERENELY_PLEASE_KEEP_GOING ?

Comment 23

[Benjamin Smedberg]

Comment on attachment 294921 [details] [diff] [review]
updated

This is fine with me... Boris, does this address your concerns adequately?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 294921 [details] [diff] [review]
updated

>Index: toolkit/components/build/nsToolkitCompsModule.cpp
>+    nsUrlClassifierDBService *inst = nsUrlClassifierDBService::GetInstance(&rv);

I have to admit to being a little confused here.  Does GetInstance() return an already addrefed pointer?  If so, should it return an already_AddRefed?

>+    /* NS_ADDREF(inst); */

What does that mean?

>+    rv = inst->QueryInterface(aIID, aResult);

Why not use CallQI here?

>Index: toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp
>+nsUrlClassifierDBService::GetInstance(nsresult *result)
> {

You didn't change this part, really, but I'm confused by the refcounting in this function.  If !sURLClassifierService, this code creates it, addrefs it, and returns it, right?  If sURLClassifierService, it gets addrefed and returned.

So in the former case we're not returning an already addrefed pointer (since releasing will make sUrlClassifierDBService stale), and in the latter case we are.  That seems very odd to me.  Am I missing something?

Comment 25

[Alexander Sack]

(In reply to comment #24)
> (From update of attachment 294921 [details] [diff] [review])
> >Index: toolkit/components/build/nsToolkitCompsModule.cpp
> >+    nsUrlClassifierDBService *inst = nsUrlClassifierDBService::GetInstance(&rv);
> 
> I have to admit to being a little confused here.  Does GetInstance() return an
> already addrefed pointer?  If so, should it return an already_AddRefed?
>

I just adapted the code that existed and tried to stick to the current ref approach ... so any ref issue that we have now here was there before - unless i miss something.

> >+    /* NS_ADDREF(inst); */
> 
> What does that mean?
> 
> >+    rv = inst->QueryInterface(aIID, aResult);
> 
> Why not use CallQI here?

i took the code from  NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR definition in nsIGenericFactory.h - http://mxr.mozilla.org/mozilla/source/xpcom/glue/nsIGenericFactory.h#447 - I didn't really question that, because I assumed that this code is widely used and would do the right magic.

> 
> >Index: toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp
> >+nsUrlClassifierDBService::GetInstance(nsresult *result)
> > {
> 
> You didn't change this part, really, but I'm confused by the refcounting in
> this function.  If !sURLClassifierService, this code creates it, addrefs it,
> and returns it, right?  If sURLClassifierService, it gets addrefed and
> returned.
> 
> So in the former case we're not returning an already addrefed pointer (since
> releasing will make sUrlClassifierDBService stale), and in the latter case we
> are.  That seems very odd to me.  Am I missing something?
> 

sounds reasonable ... so I looked for other singleton implementations. For instance we have:
http://mxr.mozilla.org/mozilla/source/netwerk/base/src/nsIOService.cpp#262

so ... looks like the GetInstance code should really do an ADDREF at the end (not just if !sURLClassifierService) - makes sense imo.

Would that be good enough?

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 294921 [details] [diff] [review]
updated

I know you just copied what the macro expands to...  I guess my problem is that it expands to fragile code.  As long as it's used via the macro it's sort of ok, but hand-writing that kind of code is something we try not to do.

Then again, all this stuff will get rewritten for moz2, so maybe we shouldn't worry.

I just double-checked the refcounting, and it looks like it's sort of OK after all.  When the last ref dies, we null out sUrlClassifierDBService.  So while it's painful to figure out why this code works it does work....

Comment 27

[Reed Loden [:reed]]

Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v  <--  nsDocShell.cpp
new revision: 1.877; previous revision: 1.876
done
Checking in toolkit/components/build/nsToolkitCompsModule.cpp;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v  <--  nsToolkitCompsModule.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.h,v  <--  nsUrlClassifierDBService.h
new revision: 1.6; previous revision: 1.5
done