Closed Bug 404634 Opened 12 years ago Closed 12 years ago

http urls don't render in TestGtkEmbed

Categories

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

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: blassey, Assigned: asac)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
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...
I've narrowed it down to between August 30 14:51:45 PDT 2007 and Sept 1 14:51:45 PDT 2007.
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...
Blocks: 384941
Flags: blocking1.9?
Keywords: regression
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
create a temp file with 0600 permissions if no profile path exists and remove it when shutting down cleanly.
Assignee: nobody → asac
Status: NEW → ASSIGNED
Attachment #290305 - Flags: review?(bzbarsky)
I can't review this code, actually.  I'm not a toolkit peer.
Attachment #290305 - Flags: review?(bzbarsky) → review?(benjamin)
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)
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 ...
Attachment #290308 - Flags: review?(benjamin)
If we have a legitimate OOM we want to stop loading the page (since we can't tell whether it's an attack).
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?
Attachment #290308 - Flags: review?(benjamin) → review-
(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.
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 
(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

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.
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.
(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?

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.
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.
But a separate I_FAILED_SERENELY_PLEASE_KEEP_GOING result would be acceptable, right?
Yes, absolutely.
Attachment #290305 - Flags: review?(benjamin) → review-
Attached patch updatedSplinter Review
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 ?
Attachment #290305 - Attachment is obsolete: true
Attachment #290308 - Attachment is obsolete: true
Attachment #290373 - Attachment is obsolete: true
Attachment #294921 - Flags: review?(benjamin)
Comment on attachment 294921 [details] [diff] [review]
updated

This is fine with me... Boris, does this address your concerns adequately?
Attachment #294921 - Flags: review?(bzbarsky)
Attachment #294921 - Flags: review?(benjamin)
Attachment #294921 - Flags: review+
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?
(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 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....
Attachment #294921 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.