Closed Bug 54349 Opened 24 years ago Closed 23 years ago

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

Categories

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

x86
Windows NT
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.8

People

(Reporter: keith.lesch, Assigned: badami)

References

()

Details

(Whiteboard: [patch needs r/sr=])

Attachments

(13 files)

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.
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.
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.
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
Please send a testcase, or at least a code snippet.
I'll get to work on a good test case and post it.
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.
Status: UNCONFIRMED → RESOLVED
Closed: 24 years ago
Resolution: --- → INVALID
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.
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.
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...
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.
Attached file The html file including 2 css sheets (deleted) —
Attached file First css sheet (deleted) —
Attached file Second css sheet (deleted) —
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.
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".
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?
Keywords: qawanted
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...
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.
Status: RESOLVED → UNCONFIRMED
QA Contact: chrisd → tever
Resolution: INVALID → ---
Summary: CSS sheets *sometimes* do not load → stylesheet do not load when in another HTTP auth realm (sometimes)
Whiteboard: need better testcases
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yes, this is the exact problem I was having. Thank you for the reproduction of my testcase.
*** Bug 104542 has been marked as a duplicate of this bug. ***
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.
Assignee: pierre → darin
Component: Style System → Networking: HTTP
Attached file TCP Log of a Mozilla session (deleted) —
not sure if i'll have time to look at this for 0.9.7 -> 0.9.8
Target Milestone: --- → mozilla0.9.8
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.
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.
Depends on: 72111
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.
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.
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 ?
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.
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.
reassigning to vinay
Assignee: darin → badami
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; } }
Attachment #61731 - Flags: needs-work+
Attached patch incorpoarating darins comments (deleted) — Splinter Review
Keywords: testcasereview
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.
Attachment #61916 - Flags: review+
Attached patch get rid of cbs (deleted) — Splinter Review
cbs not required.
Whiteboard: need better testcases
Comment on attachment 62311 [details] [diff] [review] get rid of cbs sr=darin
Attachment #62311 - Flags: superreview+
Attachment #62311 - Flags: review+
Darin, Rick One of u will need to check this in for me please.
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 24 years ago23 years ago
Resolution: --- → FIXED
This patch caused a significant leak increase. See bug 118726.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
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.
In comment 45 weka ==> weak
Keywords: patch
Whiteboard: patch needs r/sr
Whiteboard: patch needs r/sr → [patch needs r/sr=]
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)
Attachment #64724 - Flags: needs-work+
Attached patch with darins comments (deleted) — Splinter Review
rpotts - can u please do a r= on this for me ?
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
Attached patch incorporating rick's comments (deleted) — Splinter Review
Comment on attachment 66724 [details] [diff] [review] incorporating rick's comments r=rpotts@netscape.com
Attachment #66724 - Flags: review+
Comment on attachment 66724 [details] [diff] [review] incorporating rick's comments sr=darin
Attachment #66724 - Flags: superreview+
fixed-on-trunk
Status: REOPENED → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Verified fixed based on testcase in comment #19
Status: RESOLVED → VERIFIED
QA Contact: tever → junruh
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: