54349 - stylesheet do not load when in another HTTP auth realm (sometimes)

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Keith Lesch]

I am having a problem with CSS sheets loading.  I have played with them a bit 
and found situations where I can get them to load.  If I put the style sheets 
after a few javascript includes, then they will load.  If the javascript 
includes are not there, they do not load.  Or, if they are included before the 
javascript includes, then they do not load.  I am working on a test case, but am 
not sure how soon I will be able to get you something that consistently does not 
work.

Comment 1

[Keith Lesch]

FYI, I am using build 2000092608 so I am up to date as much as I can be.  My 
main problem is that I cannot get this problem to reproduce on a page with 
minimal content.  I have a large page on a protected server that is easy to 
reproduce on.  Also, a guess of mine might be that there is a relation to the 
pages being password-protected via NT basic authentication.

Comment 2

[Pierre Saslawsky]

Keith, we did have some problems when loading stylesheets in the past but they 
have all been fixed, afaik. Bug 41637 is about interactions between content and 
chrome (the content can't load a chrome stylesheet) and bug 44285 was about 
sytlesheet that are being LINKed from JavaScript. Don't miss my comment dated 
from [2000-09-11 02:35] in bug 44285, maybe you are running into the same 
problem.

Comment 3

[Keith Lesch]

I am using the LINK syntax specified at the below URL by the W3C for persistent 
style sheets.  I am trying to specify two simultaneous, and am not specifying 
the TITLE attribute.  Occasionally the final output of the page is garbled with 
only the javascript being displayed with no spaces, no HTML, no parsing.  The 
reload button usually solves that, but only applies one style sheet.  Should I 
have to specify the META default attributes to get these syle sheets to load???

http://www.w3.org/TR/REC-html40/present/styles.html#specifying-external

Comment 4

[Pierre Saslawsky]

Please send a testcase, or at least a code snippet.

Comment 5

[Keith Lesch]

I'll get to work on a good test case and post it.

Comment 6

[Pierre Saslawsky]

Closing as Invalid to lower the stats.
Keith, please reopen when you have a testcase. Even something that shows the bug 
intermittently would be nice.

Comment 7

[Keith Lesch]

No problem.  Sounds fine to me, I will re-open if/when I find a test case.  FYI, 
I have not been having problems when the page is not password-protected.  It 
seems to be associated with password protection and downloading images, css, and 
scripts that are included.  I have about 20 small images included on the pages 
and it usually gets about 1 or 2 of them.

Comment 8

[Keith Lesch]

Okay, I have a fairly straightforward set of files that I can send you that is 
not working.  However, they work fine IF they are not in a passworded directory.  
To make it even worse, I can sometimes make the browser load the style sheets 
anyway if I move them around in a different order combined with script tags in 
the header.  

Another point of note is to make sure you do not attempt to load the pages 
before password protecting them.  They will be cached and then the browser will 
load from the cache since it could not get them from the server.

Another thing that is clearly part of this issue is that merely password 
protecting the files is not enough.  You must have them in different 
subdirectories as well.  For instance in my case the css files are in /css and 
the html file is in /html.  

As for a place you can test this, I will have to set it up and that will take a 
little additional time.  I will open this bug as soon as I find a public server 
with which to do this.  NASA will not allow me to use theirs.

Comment 9

[Pierre Saslawsky]

You can already attach the files to the bug report. Maybe with another network 
configuration, we won't need to store the files in a password protected 
directory.
- How often do you see the problem?
- Do you have a network analyzer? Just to make sure that the stylesheets have 
been requested and served...

Comment 10

[Keith Lesch]

The problem happens every time in the situation I have described.  No I do not 
have a network analyzer, or at least I am not aware of it and definitely 
wouldn't know how to use it.  I will attach the files, and once again, the 
problem only occurs if they are in separate subdirectories.  I will keep my 
fingers crossed that this is not an IIS issue with Microsoft.

Comment 11

[Keith Lesch]

Attachment: The html file including 2 css sheets

Comment 12

[Keith Lesch]

Attachment: First css sheet

Comment 13

[Keith Lesch]

Attachment: Second css sheet

Comment 14

[Keith Lesch]

I did not skim down the content.  The problem will be somewhat obvious if it 
occurs, the background is white if the style sheets are not retrieved, and the 
background is yellow if they are.

Comment 15

[Keith Lesch]

Attachment: Skimmed HTML content, self-explanatory.

Comment 16

[Keith Lesch]

Sorry, I should not have been so inconsiderate not to make the HTML file 
self-explanatory.  Use the second HTML file in place of the first.  Once again, 
it will look for the css in "../css/icm.css" and "../css/help.css".

Comment 17

[Hixie (not reading bugmail)]

Ok, I can try to recreate these conditions.

Please tell me if this is correct. I need to place the files in this directory 
structure:

   .../
      html/ the html file
      css/ the two css files

So for example, in  ~/web/test/html/page.html and ~/web/test/css/style.css ?

And next I need to add a .htaccess file to password protect the directory? Is
that the CSS directory, the HTML directory, or both?

Comment 18

[Keith Lesch]

If you protect the test directory that should cover you.  Otherwise protect both 
of them.  You may have to do them individually to mimic whatever IIS does with 
windows NT.  Thanks...

Comment 19

[Hixie (not reading bugmail)]

Ok, I can reproduce it. I have recreated the scenario described here:
   http://damowmow.com/mozilla/bugs/54349/html/test.html
The username is "test" and the password "testtest".

If it works for you, empty your cache and reset your password cache and try
again a few times.

QA Reassigning to tever, since this is a networking problem. Tom: If you want
access to the directory in which I have these test files, give me a shout.

Comment 20

[Keith Lesch]

Yes, this is the exact problem I was having.  Thank you for the reproduction of 
my testcase.

Comment 21

[Marian]

*** Bug 104542 has been marked as a duplicate of this bug. ***

Comment 22

[Pierre Saslawsky]

Reassigned to Networking:HTTP.  The Authorization data is not provided to the 
host when we attempt to load the stylesheets.  I'll attach the TCP logs of 2 
sessions: one with Moz, one with IE.

Comment 23

[Pierre Saslawsky]

Attachment: TCP Log of a Mozilla session

Comment 24

[Pierre Saslawsky]

Attachment: TCP Log of an Internet Explorer session

Comment 25

[Darin Fisher]

not sure if i'll have time to look at this for 0.9.7 -> 0.9.8

Comment 26

[Vinay Badami]

Darin
there is a problem here.
When we try to fetch the css files, we get a 401. We then try to prompt the user
since the realm is different. However, this prompting does not work. This is
because, 
nsCOMPtr<nsIAuthPrompt> authPrompt = do_GetInterface(mCallbacks, &rv); 
mCallbacks never got initialized for this channel. Hence the above line comes
back with rv != 0.

This seems to happen only when nsHttpChannel::SetNotificationCallbacks is called
which happens from NS_NewURI only. 

In this case NS_NewURI was not called. Hence the problem.

Comment 27

[Darin Fisher]

vinay: yeah, i haven't figured out how to solve this problem yet.  we really
need someone more familiar with layout to help.  the problem is that the CSS
loader does not assign notificationCallbacks to the stream it is loading.  see
content/html/style/src/nsCSSLoader.cpp... look for NS_NewStreamLoader... notice
that the notificationCallbacks parameter is NULL.

there is a plan to make channel's inherit notificationCallbacks from their load
group if none is explicitly set on the channel.  i don't recall the bug number
on that one though.

this should really be marked as a dependency on that bug.

Comment 28

[Vinay Badami]

hmm - bug 72111 I guess.
So I guess u r looking for something like
    nsresult rv;
	nsIInterfaceRequestor* cbs = mCallbacks;
    nsCOMPtr<nsIAuthPrompt> authPrompt = do_GetInterface(cbs, &rv); 
    if (NS_FAILED(rv)) {
        // Let us try from the mCallbacks of the loadgroup.
        mLoadGroup->GetNotificationCallbacks(&cbs);
        authPrompt = do_GetInterface(cbs, &rv);
    }
	if (NS_FAILED(rv)) {
        NS_WARNING("notification callbacks should provide nsIAuthPrompt");
        return rv;
    }

However, this would still not gaurantee anything. Infact, I thried this out and
it still fails.
HttpChannel needs to be able to prompt. So why not register a default static
nsCOMPtr<nsIAuthPrompt> _gAuthPrompt for the nsHttpChannel class which gets
initialized at startup ? That way we remove our dependency from channels that
are initialized without notification callbacks, or with callbacks that don't
include an nsIPrompt.

I do not see any other way to gaurantee this.

Comment 29

[Darin Fisher]

the problem is that no one is setting notification callbacks on the load group.
 once this is done, however, then your patch would work.  docshell should be
sure to set notification callbacks on its load group.

Comment 30

[Vinay Badami]

Attachment: default to loadgroups else windowwatcher

We try to get a nsIAuthPrompt Interface first from the callbacks, else from
callbacks of the loadgroups. 
If both these fail, we default to using the widow-watcher service. This service
is used in other parts of the code for similar purposes.
With this do get prompted again for .css files since they come off a different
realm and all works fine.

If the approach is acceptable, would like the reviewer to peer a bit closely at

rv = wwatch->GetNewAuthPrompter(0, getter_AddRefs(authPrompt)); 
Did I get my refcounting correct here ?

Comment 31

[Darin Fisher]

vinay: this patches work, but it adds a new necko compile time dependency, which
we can avoid if we add code to the docshell to add notificationcallbacks to the
loadgroup.  can you look into doing this instead?

see docshell/base/nsDocShell.cpp... search for NS_OpenURI.  the
docshell/uriloader should ensure that the loadgroup has the docshell set as the
notificationcallbacks for the loadgroup.

Comment 32

[Vinay Badami]

Attachment: default to patchRes only and set cbs on loadgroup

Darin
Was able to get it to work setting callback on the loadgroup. 
Couple of questions/issues :
1. Is the NS_STATIC_CAST valid ?
2. Are there potentially other places - webshell etc ?
3. By defaulting to window-watcher, we can eliminate any problems with 2. Yes,
we will have to live with a compile time dependency.

Comment 33

[Darin Fisher]

reassigning to vinay

Comment 34

[Darin Fisher]

Comment on attachment 61731 [details] [diff] [review]
default to patchRes only and set cbs on loadgroup

>Index: docshell/base/nsDocShell.cpp

>     nsCOMPtr<nsILoadGroup> loadGroup(do_GetInterface(mLoadCookie));
>     NS_ENSURE_TRUE(loadGroup, NS_ERROR_FAILURE);
>+    loadGroup->SetNotificationCallbacks(NS_STATIC_CAST(nsIInterfaceRequestor *, this));

the static cast should be okay, since this is how notification callbacks are
passed
to NS_OpenURI.


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>     nsresult rv;
>-    nsCOMPtr<nsIAuthPrompt> authPrompt = do_GetInterface(mCallbacks, &rv); 
>+    nsIInterfaceRequestor* cbs = mCallbacks;
>+    nsCOMPtr<nsIAuthPrompt> authPrompt = do_GetInterface(cbs, &rv); 
>     if (NS_FAILED(rv)) {
>-        NS_WARNING("notification callbacks should provide nsIAuthPrompt");
>-        return rv;
>+        // Let us try from the mCallbacks of the loadgroup.
>+        mLoadGroup->GetNotificationCallbacks(&cbs);
>+        authPrompt = do_GetInterface(cbs, &rv);
>+        if (NS_FAILED(rv)) {
>+            // Unable to prompt -- return
>+            NS_WARNING("notification callbacks should provide nsIAuthPrompt");
>+            return rv;

this code leaks a nsIInterfaceRequestor... use a COMPtr for the cbs parameter
to
avoid the leak.

like this:

nsCOMPtr<nsIAuthPrompt> authPrompt( do_GetInterface(mCallbacks, &rv) );
if (NS_FAILED(rv)) {
    // there may not be a load group!
    if (mLoadGroup) {
	nsCOMPtr<nsIInterfaceRequestor> cbs;
	rv = mLoadGroup->GetNotificationCallbacks(getter_AddRefs(cbs));
	// the load group may not have any notification callbacks
	if (NS_SUCCEEDED(rv))
	    authPrompt = do_GetInterface(cbs, &rv);
    }
    if (NS_FAILED(rv)) {
	// unable to prompt -- return
	NS_WARNING("...");
	return rv;
    }
}

Comment 35

[Vinay Badami]

Attachment: incorpoarating darins comments

Comment 36

[Darin Fisher]

Comment on attachment 61916 [details] [diff] [review]
incorpoarating darins comments

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+    nsIInterfaceRequestor* cbs = mCallbacks;
>+    nsCOMPtr<nsIAuthPrompt> authPrompt(do_GetInterface(cbs, &rv)); 

so, why bother copying mCallbacks to cbs at all?  seems unnecessary.

other than that, this patch looks fine.  fix this and r=darin.

Comment 37

[Vinay Badami]

Attachment: get rid of cbs

cbs not required.

Comment 38

[Darin Fisher]

Comment on attachment 62311 [details] [diff] [review]
get rid of cbs 

sr=darin

Comment 39

[rpotts (gone)]

Comment on attachment 62311 [details] [diff] [review]
get rid of cbs 

r/sr=[e-mail]

Comment 40

[Vinay Badami]

Darin, Rick
One of u will need to check this in for me please.

Comment 41

[Darin Fisher]

fixed-on-trunk

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch caused a significant leak increase.  See bug 118726.

Comment 43

[Darin Fisher]

reopening this bug... i had to back out the nsDocShell.cpp change to fix leak
that resulted from this patch.  there was a cycle between the docshell and it's
loadgroup.  the changes to nsHttpChannel are still in the tree.

Comment 44

[Andrew Hagen]

Editing for grammar.

Old Summary: stylesheet do not load when in another HTTP auth realm (sometimes)

New Summary: stylesheets do not load when in another HTTP auth realm (sometimes)

Comment 45

[Vinay Badami]

Attachment: added a proxy class to break the cyclic reference which was causing a leak

Defined a InterfaceRequestorProxy class to hold a weka ref to the docshell.
This enables breaking of the cyclic reference between the docshell and the
loadgroup caused as a result of the previous patch.

Comment 46

[Vinay Badami]

In comment 45
weka ==> weak

Comment 47

[Darin Fisher]

Comment on attachment 64724 [details] [diff] [review]
added a proxy class to break the cyclic reference which was causing a leak

>Index: base/nsDocShell.cpp

> nsDocShellFocusController nsDocShellFocusController::mDocShellFocusControllerSingleton;
> 
>+
>+
>+
>+

please don't add extra whitespace like this.  i don't believe it is consistent
with
the existing formating style of nsDocShell.cpp


>+
>+

how about just one line break here?  "when in rome, stay in rome."


>+//*****************************************************************************
>+// nsDocShell::InterfaceRequestorProxy
>+//*****************************************************************************  
>+nsDocShell::InterfaceRequestorProxy::InterfaceRequestorProxy(nsIInterfaceRequestor* p) : _mWeakPtr(nsnull)
>+{
>+    NS_INIT_REFCNT();
>+    if (p) {
>+        _mWeakPtr = getter_AddRefs(NS_GetWeakReference(p));
>+    }
>+}
>+ 
>+nsDocShell::InterfaceRequestorProxy::~InterfaceRequestorProxy()
>+{
>+    _mWeakPtr = nsnull;
>+}

again, stylistically this does not match existing code.  please be consistent.
leading underscores should be eliminated.  also, nsWeakPtr does not require an
explicit initialization line in the containing class's constructor.

>+
>+//*****************************************************************************
>+// nsDocShell::InterfaceRequestorProxy::nsISupports 
>+// nsDocShell::InterfaceRequestorProxy::nsIInterfaceRequestor
>+//*****************************************************************************

i'd do away with these extra comments myself since the impl is so simple and
also because
this is just an inner class of nsDocShell and these divider comments seem to be
dividing major
sections of the docshell implementation only.

>Index: base/nsDocShell.h

>+    private:
>+    nsWeakPtr _mWeakPtr;
>+    };

please indent this member variable declaration.

otherwise, sr=darin (please submit a revised patch)

Comment 48

[Vinay Badami]

Attachment: with darins comments

Comment 49

[Vinay Badami]

rpotts - can u please do a r= on this for me ?

Comment 50

[rpotts (gone)]

hey vinay,

i'd put the code to set the notification callbacks on the loadgroup in
nsDocShell::SetLoadCookie() rather than nsDocShell::CreateContentViewer().

This way, the notification callbacks are only setup once per docshell, rather
than on every page load...  

-- rick

Comment 51

[Vinay Badami]

Attachment: incorporating rick's comments

Comment 52

[rpotts (gone)]

Comment on attachment 66724 [details] [diff] [review]
incorporating rick's comments

r=[e-mail]

Comment 53

[Darin Fisher]

Comment on attachment 66724 [details] [diff] [review]
incorporating rick's comments

sr=darin

Comment 54

[Darin Fisher]

fixed-on-trunk

Comment 55

[John Unruh]

Verified fixed based on testcase in comment #19