549241 - Channel classifier should run in chrome process

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Michal Novotny [:michal]]

We need to run channel classifier (implemented by nsClassifierCallback) in chrome process because otherwise we would need to expose cache API and nsIURIClassifier through IPC. The nsClassifierCallback is now used in nsDocShell, nsObjectLoadingContent, nsScriptLoader, CSSLoaderImpl and nsDOMWorkerScriptLoader. All scenarios are the same, i.e. create a channel, call AsyncOpen() on it and pass that channel to nsIChannelClassifier::Start(). Creating a simple protocol that would implement all methods from nsIChannelClassifier won't work because:

1) We would need a mechanism (as described in bug514705 comment #8) of passing a pointer to the channel from parent to child to be able to pass it back to the nsIChannelClassifier::Start().
2) Without some synchronization it could happen that channel loading would be already completed before calling nsIChannelClassifier::Start() in the parent process. The same problem is with nsIChannelClassifier::OnRedirect() where even now is the synchronization a little bit tricky.

A simple possible solution is to create the channel classifier in the protocol handler when the channel is created or in the channel in AsyncOpen() method. Also the nsClassifierCallback functionality could be completely implemented directly in the channel.

Any opinions on this?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> passing a pointer to the channel from parent to child to be able to pass it
> back to the nsIChannelClassifier::Start().

If all channels, not just http, lived in the parent, couldn't we just pass the child channel to the child classifier and have it get converted to the parent actor at the ipdl boundary?

But yes, maybe classification should simply happen as part of the http protocol...

Comment 2

[Michal Novotny [:michal]]

(In reply to comment #1)
> If all channels, not just http, lived in the parent, couldn't we just pass the
> child channel to the child classifier and have it get converted to the parent
> actor at the ipdl boundary?

I need a pointer to nsIChannel on parent side in ChannelClassifierParent. Who will do the conversion when I pass the HttpChannelChild to ChannelClassifierChild on the child side?

Comment 3

[Boris Zbarsky [:bzbarsky]]

If you pass an HttpChannelChild on the child side, you get an HttpChannelParent on the parent side.  I believe the ipdl guts make that work out.

Comment 4

[Benjamin Smedberg]

We talked on IRC about just setting a flag on the channel when you first create it which means "please classify this channel". In this case you can avoid passing the channel as an argument anywhere and avoid all the race issues.

Comment 5

[Michal Novotny [:michal]]

I was just curious about the conversion. If Boris or Christian isn't against that, I would move whole classifying into the parent.

Comment 6

[Michal Novotny [:michal]]

Attachment: patch v1

This patch moves URI classification to the channel.

- now only HTTP channel is doing the classification. Which other channels should do it?
- interface nsIChannelClassifier was removed since it is no longer needed
- nsChannelClassifier (was nsClassifierCallback) doesn't handle redirects anymore, now this is done implicitly

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 432677 [details] [diff] [review]
patch v1

+++ b/content/base/src/nsObjectLoadingContent.cpp
+                     nsIChannel::LOAD_CALL_CONTENT_SNIFFERS ||
+                     nsIChannel::CLASSIFY_URI);

you might want to use | instead of ||

same in some other files...

if only HTTP uses this flag, it should probably move to nsIHttpChannel instead of living on nsIChannel

+    cacheEntry->SetMetaDataElement("docshell:classified",

this shouldn't use a docshell: prefix if it lives in necko

+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
+        if (!classifier) {
+            Cancel(NS_ERROR_OUT_OF_MEMORY);
+        }

You also want to return here, otherwise your ->Start call below will fail.

Comment 8

[Michal Novotny [:michal]]

Attachment: patch v2 - updated according to comment #7

(In reply to comment #7)
> if only HTTP uses this flag, it should probably move to nsIHttpChannel instead
> of living on nsIChannel

Actually that was my question. What channels should do the classification? The current code classifies also another protocols (e.g. FTP), but do we want this? URI classifier database doesn't contain a protocol info, right? For example firefox shows a warning page when I try to load ftp://www.mozilla.com/firefox/its-a-trap.html

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

I guess we do want to classify FTP URIs, otherwise that would be an easy way around the malware check, which seems undesirable. Should probably put that in nsBaseChannel.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 436617 [details] [diff] [review]
patch v2 - updated according to comment #7

I'm marking this r- for now because I do think FTP needs this as well

Comment 11

[Michal Novotny [:michal]]

Attachment: patch v3 - classification added to nsBaseChannel

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 438466 [details] [diff] [review]
patch v3 - classification added to nsBaseChannel

+++ b/netwerk/base/public/nsIChannel.idl

+    const unsigned long CLASSIFY_URI = 1 << 22;


this should be named LOAD_CLASSIFY_URI for consistency with the other load flags


+++ b/netwerk/base/src/nsChannelClassifier.h
+    ~nsChannelClassifier() {}

Nonvirtual destructors should be private


Any particular reason why for HTTP, you run the classifier after the request has been added to the loadgroup, but for basechannel you do it before? I'd suggest doing it after in both cases.

Comment 13

[Michal Novotny [:michal]]

Attachment: patch v4

Comment 14

[Josh Matthews [:jdm]]

Drive-by comment: null checks after new are no longer required.

Comment 15

[Jason Duell]

Comment on attachment 442085 [details] [diff] [review]
patch v4

We're already got biesi signed off on this patch, so we don't need bz to review.  I'll take a look at it or find someone else.

Comment 16

[Michal Novotny [:michal]]

I thought that this patch could go into mozilla-central repository. That's why I asked for super-review.

Comment 17

[Jason Duell]

Biesi is a super-reviewer, and he approved v3 of your patch.  I'm assuming v4 contains his fixes, so all we need now is a regular review.  Am I missing something?

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 442085 [details] [diff] [review]
patch v4

Per todays necko meeting this doesn' need Jason's review, marking sr+ (sr=biesi)

Comment 19

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/d7ba6aef21eb

Yes, I messed up the patch attribution.  Oops.