7.78 KB, patch
|Details | Diff | Splinter Review|
5.78 KB, patch
Darin Fisher: superreview+
|Details | Diff | Splinter Review|
585 bytes, text/html
226 bytes, text/html
Necko clients must explicitly set the Referer line before reading from an HTTP channel. The Referer is otherwise unknown to Necko. ->layout
please wontfix this. If someone objects please cite an RFC as grounds.
RFC 2068 HTTP/1.1 January 1997 14.37 Referer The Referer[sic] request-header field allows the client to specify, for the server's benefit, the address (URI) of the resource from which the Request-URI was obtained (the "referrer", although the header field is misspelled.) The Referer request-header allows a server to generate lists of back-links to resources for interest, logging, optimized caching, etc. It also allows obsolete or mistyped links to be traced for maintenance. The Referer field MUST NOT be sent if the Request-URI was obtained from a source that does not have its own URI, such as input from the user keyboard. Referer = "Referer" ":" ( absoluteURI | relativeURI ) Example: Referer: http://www.w3.org/hypertext/DataSources/Overview.html If the field value is a partial URI, it SHOULD be interpreted relative to the Request-URI. The URI MUST NOT include a fragment.
I'm guessing the DOM folks are responsible for loading scripts. Reassigning.
IE seems to send a referrer when loading scripts but NS 4.x doesn't so unless someone can give a good reason as to why this is important I won't spend any time on fixing this. Marking WONTFIX for now, please reopen if there's a strong case for fixing this (I didn't see anything in the text from the RFC that says a client *must* send a referrer every time something is requested) or if someone wants to take this task off my list and fix it.
Reopening. This is important for the same reason as referers on images are important -- to prevent resource/cycle theft, where another website hosts just the content but points to scripts on other hosts. jst: Feel free to move this to firstname.lastname@example.org if you don't want to fix it, but this is a valid RFE and there is no reason why we would _not_ want to have this behaviour in an ideal world.
Ian, Thank you. Terje
Valid enhancement request. Setting status to New.
Ugh. setting to new for real. Sorry for the spam...
Ok, fair enough and I could even fix this given the time to do so but I'm reassigning to email@example.com for now.
QA contact Update
See also bug 74284, referer not send to inline images.
*** Bug 80390 has been marked as a duplicate of this bug. ***
clarified summary for searchability
Updating QA contact to Shivakiran Tummala.
I even saw some sites blocking NS 6.0 because it did not support referer, can't find the sites now to check if they still block us... I am sure they would also insist this bug be fixed.
This works for images, so restricting summary.
The attached patch sets the referrer url when loading scripts and, consequently, fixes http://thedarkexile.dpn.ch so that it loads properly in Mozilla. I still have to run Vidur's script loader tests to make sure I didn't break anything. Will then ask for a review and super-review. If any of you can spot obvious problems in the attached patch, please let me know. Thanks!
It would help if some of you could test out this patch and report any problems you see. Thanks!
Nisheeth - This has a mozilla1.2 in the keywords . . . Is this really a stop ship for 0.9.4. If so, please mark it as nsbranch+. If not, pls move it out to another milestone.
Moving, we really should get this done with as soon as Nisheeth comes back from vacation.
Btw since http://thedarkexile.dpn.ch moved, you can test the fix at http://nightwatchers.dpn.ch or directly at http://www.magelo.com I really hope this fix will be commited to the main tree for the 0.9.6 Thanks in advance.
Johnny, could you please take care of this? Nisheeth made a patch, but he said he run into some assertion when testing.
*** Bug 110040 has been marked as a duplicate of this bug. ***
Guess we missed 0.9.6 ? :( I really need this bug fixed asap please, this is far from an enhancement for me, i cant migrate my dev environement under linux, and users would like to be able to use Mozilla/Netscape on their website. Best regards.
16 years ago
The assertion Nisheeth refers to is probably this thread safety assertion: ###!!! ASSERTION: nsScriptLoadRequest not thread-safe: 'owningThread == NS_CurrentThread()', file nsDebug.cpp, line 528 ###!!! Break: at file nsDebug.cpp, line 528 Changing nsScriptLoadRequest to use NS_IMPL_THREADSAFE_ISUPPORTS0 makes it go away. As advertised, the patch makes http://nightwatchers.dpn.ch work. However, pages from magelo.com itself (http://www.magelo.com, http://www.magelo.com/eq_view_profile.html?num=7598, etc) only load partially (a background appears, which is more than happens without the patch, but nothing else shows up). I'm not sure if this is an issue on our end or magelo's.
Would it not be cleaner to simply add the referrer and referrer flags as optinonal arguments to NS_NewStreamLoader() and deal with the referrer there in stead of in the script loader? That would be a far smaller change I would think. I'll see if I can hack that up...
Thanks for the testcase, Emmanuel. Darin, Gagan, would you review this stream loader change please?
Comment on attachment 63399 [details] [diff] [review] Add referrer arguments to NS_NewStreamLoader() r=gagan
Comment on attachment 63399 [details] [diff] [review] Add referrer arguments to NS_NewStreamLoader() >Index: content/base/src/nsScriptLoader.cpp >-NS_IMPL_ISUPPORTS0(nsScriptLoadRequest) >+ >+// The nsScriptLoadRequest is passed as the context to necko, and thus >+// it needs to be threadsafe. Necko won't do anything with this >+// context, but it will AddRef and Release it on other threads. >+ >+NS_IMPL_THREADSAFE_ISUPPORTS0(nsScriptLoadRequest) are you sure? this shouldn't be the case. sr=darin
Yeah, I saw the assertions, some proxying code was notifying some code about something which ended up QI'ing the nsScriptLoaderRequest on a thread other than the main thread. IIRC it only happened in an error case, so it doesn't happen all the time.
would be really great if you could attach the stack trace to that assertion. it really shouldn't be happening. necko consumers aren't supposed to know about multithreading.
Here's a stack for the thread-safety assertions: nsDebug::Assertion(const char * 0x02169cac, const char * 0x1010e32c, const char * 0x1010e304, int 528) line 290 + 13 bytes NS_CheckThreadSafe(void * 0x00482e00, const char * 0x02169cac) line 528 + 34 bytes nsScriptLoadRequest::AddRef(nsScriptLoadRequest * const 0x045e59b0) line 102 + 55 bytes nsCOMPtr<nsISupports>::nsCOMPtr<nsISupports>(nsISupports * 0x045e59b0) line 787 nsARequestObserverEvent::nsARequestObserverEvent(nsIRequest * 0x045e8ad4, nsISupports * 0x045e59b0) line 99 + 37 bytes nsOnStartRequestEvent::nsOnStartRequestEvent(nsRequestObserverProxy * 0x045ee640, nsIRequest * 0x045e8ad4, nsISupports * 0x045e59b0) line 139 + 23 bytes nsRequestObserverProxy::OnStartRequest(nsRequestObserverProxy * const 0x045ee640, nsIRequest * 0x045e8ad4, nsISupports * 0x045e59b0) line 252 + 39 bytes nsStreamListenerProxy::OnStartRequest(nsStreamListenerProxy * const 0x045eb040, nsIRequest * 0x045e8ad4, nsISupports * 0x045e59b0) line 244 nsFileTransport::Process(nsIProgressEventSink * 0x045eb0f0) line 674 + 96 bytes nsFileTransport::Run(nsFileTransport * const 0x045e8ad8) line 639 nsThreadPoolRunnable::Run(nsThreadPoolRunnable * const 0x0457e230) line 904 + 12 bytes nsThread::Main(void * 0x0457e1e0) line 120 + 26 bytes
Fix checked in. Darin, please reopen or file new bugs if you think necko shouldn't cause the above assertion to fire when the nsScriptLoadRequest is addreffed/released by necko, for now nsScriptLoaderRequest has threadsafe AddRef/Release methods.
hrm.. thx for the stack trace.. looks like the nsInputStreamChannel requires the nsISupports context parameter to support threadsafe addref/release. it really doesn't have to be that way. see bug 118820.
*** Bug 128310 has been marked as a duplicate of this bug. ***