|
Description
Joseph Pecoraro
2010-12-20 16:22:59 PST
Created attachment 77064 [details]
[PATCH] Part 1: Change Code Generation for InspectorBackendDispatcher
Minor change. This changes:
#include <InspectorController.h>
To:
#include "InspectorController.h"
So that the file can be included later in WebKit/mac. Let
me know if there is a different, preferred way.
Created attachment 77065 [details]
[PATCH] Part 2: Update Styles for when the Inspector Web Page is Used Remotely
If the old CSS rules applied, then the #toolbar background would be transparent.
This seems to work fine and is preferred for the inspector as a custom NSWindow,
but as a web page that makes the toolbar background white. This makes it so
the "transparent !important" rule doesn't take affect when used remotely.
Created attachment 77066 [details]
[PATCH] Part 3: Add WebInspectorServer, Connection, and Remote Channel
This is the bulk of the work. It adds the Server, Connection, and Channel classes.
It modifies the Client to work with a remote connection, but nothing is turned
on in this patch. This is just the support.
Created attachment 77067 [details]
[PATCH] Part 4: Add Preference for Port # and to Enable/Disable WebInspectorServer
Disabled by default. Port 9999 by default. (No reason, just what I tested with).
Concerns / Comments / FIXMEs off of the top of my head:
• Improve Listing Page:
- Cull uninteresting webviews: Currently all WebViews show up in the listing.
That includes Inspector web views and if you use the browser while remote
debugging, you can even try to debug your own web view (doh).
- Should WebView's opt-out or opt-in to remote debugging? I haven't decided yet.
• Privacy / Security:
- Respect private browsing, and disable this feature.
- Possibly limit to just localhost connections early on?
- We need to sanitize, or change, what we output on the listing page. Currently
this dumps the document title / url on the listing page in a JavaScript string.
This means injection of a single quote, or </script> may break the listing page.
FIXME is included.
• Remote versus Local Debugger
- Currently the first inspector session to start has control. It may make more
sense for a local session to always take priority, and to take control away
from a remote session. If that is desired, then there would need to be a
small changes otherwise. Currently, in that scenario, "open inspector" locally
just trys to "bringToFront" the remote inspector.
• Enable / Disable a different way?
- Currently, I am using defaults and preferences changed to enable the server.
However, apparently this caused "Safari Webpage Preview Fetcher" to try and
create a web server, since it also uses Safari's preferences and handler?
• Usability:
- Discoverability of the feature, should probably be done at the browser level.
- Sending error messages back to the remote side is not very good. For instance,
if there is already a local debug session and a remote debug session is
attempted. We prevent the remote session from starting, but the remote user gets
no warning, just a mostly blank page. Many other error conditions have no
user warning on the remote side.
- "Slow scripts" executed on a remote frontend pop up a modal dialog on the
backend page. This would make using the debugger impossible! See bug 46247,
and bug 34303, and probably others.
- There can be many other improvements. Such as sending screenshots or "fake
rendering" what the page probably looks like in a remote debugger. Etc.
Oh, I left in a lot of NSLog's. I guess they could all be removed, they were mostly for debugging, but they also make following the code easier. I didn't think it was worth creating a new LogChannel just for this, but that is a possible idea. Created attachment 77073 [details]
[IMAGE] Listing Page Sample
Sample image of the listing page. Since its tough to determine from the patch.
Created attachment 77075 [details]
[IMAGE] Remote Session Sample
Image of a Remote Session. Once enabled.
To enable on Mac with Safari:
shell> defaults write com.apple.safari WebKitInspectorServerEnabled -bool YES
shell> run-safari
2010-12-20 19:42:25.230 Safari[37319:a07] WebInspectorServer: Started
2010-12-20 19:42:25.232 Safari[37319:a07] WebInspectorServer: Address (localhost)
2010-12-20 19:42:25.232 Safari[37319:a07] WebInspectorServer: Port (9999)
...
Then you can connect to the machine on that port. For example for a machine
named "foo.example.com" you could connect to "foo.example.com:9999" to
get the listing page. Clicking one of the links would load a remote session for
that page.
Attachment 77066 [details] did not build on mac: Build output: http://queues.webkit.org/results/7242074 Attachment 77067 [details] did not build on mac: Build output: http://queues.webkit.org/results/7296083 > [PATCH] Part 1: Change Code Generation for InspectorBackendDispatcher
Looks Good To Me
> Part 2: Update Styles for when the Inspector Web Page is Used Remotely
Looks good to me
> [PATCH] Part 4: Add Preference for Port # and to Enable/Disable WebInspectorServer > > To enable on Mac with Safari: > > shell> defaults write com.apple.safari WebKitInspectorServerEnabled -bool YES > shell> run-safari > 2010-12-20 19:42:25.230 Safari[37319:a07] WebInspectorServer: Started > 2010-12-20 19:42:25.232 Safari[37319:a07] WebInspectorServer: Address (localhost) > 2010-12-20 19:42:25.232 Safari[37319:a07] WebInspectorServer: Port (9999) Looks like you are enabling remote debugging at the system level. I think in that case it'd be better to bind the socket only to localhost. Otherwise it will be a security hole. (In reply to comment #14) > Looks like you are enabling remote debugging at the system level. > I think in that case it'd be better to bind the socket only to localhost. > Otherwise it will be a security hole. That is something I've considered. However are there any specific concerns? Limiting remote debugging to localhost really limits its usefulness. My idea is that users would turn such a feature on and off as needed, and not leave it on all the time. And if they turn it on, it would be so they could debug from a different machine, not the local machine. But if the desire is to get the feature tested more thoroughly I would agree with that. (In reply to comment #15) > (In reply to comment #14) > > Looks like you are enabling remote debugging at the system level. > > I think in that case it'd be better to bind the socket only to localhost. > > Otherwise it will be a security hole. > > That is something I've considered. However are there any specific concerns? > Limiting remote debugging to localhost really limits its usefulness. My idea > is that users would turn such a feature on and off as needed, and not leave > it on all the time. And if they turn it on, it would be so they could debug > from a different machine, not the local machine. But if the desire is to get > the feature tested more thoroughly I would agree with that. I mean the following use case: 1) the developer turn on this flag and forget about it forever; 2) someone with help of a port scanner finds such safari and steals cookies etc directly from the browser; for cross machine debugging you can use ssh port forwarding etc. Comment on attachment 77067 [details] [PATCH] Part 4: Add Preference for Port # and to Enable/Disable WebInspectorServer View in context: https://bugs.webkit.org/attachment.cgi?id=77067&action=review Looks good, unofficial r+. Might want to fix the build issues though. > WebKit/mac/ChangeLog:18 > + (+[WebPreferences initialize]): default Enabled to NO, Port to 9999. Seems like this should start with a capital. > WebKit/mac/ChangeLog:27 > + (-[WebView _preferencesChangedNotification:]): on preference change, update the WebInspectorServer as needed. Sentence should start with a capital here too. (In reply to comment #3) > Created an attachment (id=77066) [details] > [PATCH] Part 3: Add WebInspectorServer, Connection, and Remote Channel > > This is the bulk of the work. It adds the Server, Connection, and Channel classes. > It modifies the Client to work with a remote connection, but nothing is turned > on in this patch. This is just the support. + // Stringify the data. + CFDataRef frameData = CFDataCreateWithBytesNoCopy(kCFAllocatorDefault, frameStart + 1, frameRange.length - 2, kCFAllocatorNull); + NSString *decodedString = [[NSString alloc] initWithData:(NSData *)frameData encoding:NSUTF8StringEncoding]; Where is decodedString released? I also believe ServerListing has to be prefixed to avoid class name conflicts. > + // Stringify the data. > + CFDataRef frameData = CFDataCreateWithBytesNoCopy(kCFAllocatorDefault, frameStart + 1, frameRange.length - 2, kCFAllocatorNull); > + NSString *decodedString = [[NSString alloc] initWithData:(NSData *)frameData encoding:NSUTF8StringEncoding]; > > Where is decodedString released? Good catch. > I also believe ServerListing has to be prefixed to avoid class name conflicts. I don't know the issues surrounding this. Could you (or someone) explain the details around this so I can prevent doing this in the future. I will put part 5 up later today that allows WebViews to opt-out of being
listed for remote inspector. However, the following situation can occur,
which is difficult to handle:
1. A user opens a browser which has a remote inspector server running.
2. There is one tab open, on example.com.
3. User navigates that tab to the remote inspector listing page.
3.5 The listing page generates a response from the list of WebViews and
lists "example.com" as the only result. The response is sent.
4. The listing page is loaded in the WebView, and there is a listing for
the existing WebView.
5. A WebView attempting to inspect itself causes cyclic problems and
should not be allowed.
In short, this is "Tab A" trying to remotely inspect "Tab A", which is
bad. Normal usage expects "Tab A" to remotely inspect "Tab B". And, although
it is possible, "Tab C" could then remotely inspect "Tab A" which is remotely
inspecting "Tab B", it doesn't make sense from a usability standpoint.
For those reasons, I'd like to prevent listing WebViews that are in a "remote"
inspector mode. The only way I can think of doing this currently is when
generating the listing page, check each WebView to see if they are in the
process of loading (or making a request) to something equivalent to
"localhost:port" which is the inspector server running in the browser.
This is very dirty.
Something that would make this easier, is to move remote inspector sessions
to a different scheme. Something like inspector://. Now, it would be much
easier to check if a WebView is "in a remote inspector mode" and should
not be listed. The inspector:// scheme would simply act like http://, but
its presence is much easier to detect.
Does this sound like a good idea?
An alternative solution would be to have a new dedicated window for remote
inspection, that has opt-out of being listed. Much like the existing local
inspector. We could add UI to the inspector that would allow it to provide
a listing of other pages it could switch to inspect.
Created attachment 79340 [details]
Part 5: Allow WebViews to Disallow Remote Inspection
I've rebased the patches locally, but since there hasn't been much review there hasn't
really been a need to update parts 3/4. Here is part 5 allowing a WebView to opt-out
of being listed for remote inspection.
Created attachment 79448 [details]
Part 6: Disallow Remote Inspection on WebView when Private Browsing is Enabled
Respect Private Browsing, and potentially other factors in the future, for
listing (and remotely inspecting) WebViews.
Comment on attachment 79448 [details]
Part 6: Disallow Remote Inspection on WebView when Private Browsing is Enabled
This looks good. Are there any other reasons canBeRemotelyInspected would return false that we could want in the future? Is it worth putting a FIXME there?
Comment on attachment 79340 [details] Part 5: Allow WebViews to Disallow Remote Inspection View in context: https://bugs.webkit.org/attachment.cgi?id=79340&action=review Patch looks good. > Source/WebKit/mac/WebInspector/WebInspectorServer.mm:184 > + if (!it->second->allowsRemoteInspection()) If we're using it->second multiple times, is it worth it to pull it out into a local variable? > Source/WebKit/mac/WebInspector/WebInspectorServerConnection.mm:395 > + NSLog(@"WebInspectorServerConnection: WebView does not allow remote inspection."); Is it useful to print any information about the WebView in this log? Comment on attachment 79340 [details] Part 5: Allow WebViews to Disallow Remote Inspection View in context: https://bugs.webkit.org/attachment.cgi?id=79340&action=review Patch looks good. > Source/WebKit/mac/WebInspector/WebInspectorServer.mm:184 > + if (!it->second->allowsRemoteInspection()) If we're using it->second multiple times, is it worth it to pull it out into a local variable? > Source/WebKit/mac/WebInspector/WebInspectorServerConnection.mm:395 > + NSLog(@"WebInspectorServerConnection: WebView does not allow remote inspection."); Is it useful to print any information about the WebView in this log? Comment on attachment 79340 [details] Part 5: Allow WebViews to Disallow Remote Inspection View in context: https://bugs.webkit.org/attachment.cgi?id=79340&action=review Patch looks good. > Source/WebKit/mac/WebInspector/WebInspectorServer.mm:184 > + if (!it->second->allowsRemoteInspection()) If we're using it->second multiple times, is it worth it to pull it out into a local variable? > Source/WebKit/mac/WebInspector/WebInspectorServerConnection.mm:395 > + NSLog(@"WebInspectorServerConnection: WebView does not allow remote inspection."); Is it useful to print any information about the WebView in this log? (In reply to comment #23) > (From update of attachment 79448 [details]) > This looks good. Are there any other reasons canBeRemotelyInspected would return > false that we could want in the future? Is it worth putting a FIXME there? See: https://bugs.webkit.org/show_bug.cgi?id=51364#c20 A FIXME is a good idea depending on our approach in that bug. (In reply to comment #24) > > Source/WebKit/mac/WebInspector/WebInspectorServer.mm:184 > > + if (!it->second->allowsRemoteInspection()) > > If we're using it->second multiple times, is it worth it to pull it out into a local variable? Sure, I can do that. > > Source/WebKit/mac/WebInspector/WebInspectorServerConnection.mm:395 > > + NSLog(@"WebInspectorServerConnection: WebView does not allow remote inspection."); > > Is it useful to print any information about the WebView in this log? Good point. The others all print the pageId when referring to a Page / WebView, so I will change this to: NSLog(@"WebInspectorServerConnection: WebView for Page (%d) does not allow remote inspection.", pageId); Also, these logs should eventually turn into error messages sent to the front-end so that is a good idea. See: https://bugs.webkit.org/show_bug.cgi?id=51364#c5 Comment on attachment 77066 [details]
[PATCH] Part 3: Add WebInspectorServer, Connection, and Remote Channel
A few general comments based on an extremely quick glance:
1) Why is all this code up in WebKit? WebKit should ideally be a thin API layer on top of WebCore.
2) Can we make the code more cross-platform friendly? I.e., use C++ instead of ObjC, hide the CF use inside CF-specific files, etc.
3) Is there code we should be sharing with WebCore for the WebSocket-y stuff?
(In reply to comment #28) > (From update of attachment 77066 [details]) > A few general comments based on an extremely quick glance: > > 1) Why is all this code up in WebKit? WebKit should ideally be a thin API layer on top of WebCore. > 2) Can we make the code more cross-platform friendly? I.e., use C++ instead of ObjC, hide the CF use inside CF-specific files, etc. > 3) Is there code we should be sharing with WebCore for the WebSocket-y stuff? 4) It would be nice if we could use WebKit2's CoreIPC layer for handling the IPC. Doing that would require moving CoreIPC out of WebKit2 and into some lower level (which we want to do eventually anyway). (In reply to comment #28) > (From update of attachment 77066 [details]) > A few general comments based on an extremely quick glance: These are all very good comments. I'll post some feedback for each. But I think its pretty clear that a WebCore implementation of a WebSocket server is desired. > 1) Why is all this code up in WebKit? WebKit should ideally be a thin API layer on top of WebCore. I think it is totally preferred. I thought about that early on, but in my haste to get a working version of this out and not sit on my hands, I put it into a FIXME and produced these patches. > Patch 3, WebKit/mac/WebInspector/WebInspectorServerConnection.mm: > // FIXME: Maybe this be folded into WebCore, abstracted on top of /platform/network/SocketStreamHandle. > // Moving it into WebCore would help make this maintainable and identical across platforms, as well > // as making it easier to keep in sync with WebCore's client side WebSocket implementation. A couple of other reasons I went forward with this WebKit approach were: 1. That is what Chromium (outside of WebKit) and Qt (inside WebKit/qt) have done. 2. I have very little experience with cross platform code. I felt if I could get something working first for Mac, it would be easier to then move / port that to WebCore, without originally putting it in WebCore and potentially breaking other ports or superceeding their implementations. Stepping back, #2 could be based on false assumptions I have. > 2) Can we make the code more cross-platform friendly? I.e., use C++ instead of ObjC, hide the CF use inside CF-specific files, etc. Yes, the model for that would be WebCore/platform/network/SocketStreamHandle, and other WebSocket client code which is a C++ facade for cross platform implementations. > 3) Is there code we should be sharing with WebCore for the WebSocket-y stuff? Absolutely. WebCore/websockets/WebSocketChannel.h comes to mind for framing, sending and receiving messages. Few functions are identical (`setChallengeNumber`) but there are numerous aspects of the Web Socket server implementation in this patch that must be kept in sync with WebCore's Web Socket client implementation. Esp with all the changes to the Web Socket drafts. Namely the Handshake, Message Framing, and Headers. Having these side by side in WebCore would good for all clients and might make testing / development easier having both sides at your disposal (and could lead to subtle bugs with other implementations). > 4) It would be nice if we could use WebKit2's CoreIPC layer for handling the IPC. Doing that would require moving CoreIPC out of WebKit2 and into some lower level (which we want to do eventually anyway). The only IPC here goes over HTTP requests. I'm not as familiar with WebKit2 as I would like to be, but I'm not sure this approach would benefit. Does the CoreIPC layer adapt to network communication between processes, or do you mean that on the same machine the network could be avoided? Right now this works so that your browser can inspect a webview on another machine's browser (X) by "downloading" the Web Inspector frontend (html/css/js) from X and communicating to X via Web Sockets. (In reply to comment #30) > (In reply to comment #28) > > (From update of attachment 77066 [details] [details]) > > A few general comments based on an extremely quick glance: > > These are all very good comments. I'll post some feedback for each. But I think its > pretty clear that a WebCore implementation of a WebSocket server is desired. > > > > 1) Why is all this code up in WebKit? WebKit should ideally be a thin API layer on top of WebCore. > > I think it is totally preferred. I thought about that early on, but in my haste to get a working > version of this out and not sit on my hands, I put it into a FIXME and produced these patches. > > > Patch 3, WebKit/mac/WebInspector/WebInspectorServerConnection.mm: > > // FIXME: Maybe this be folded into WebCore, abstracted on top of /platform/network/SocketStreamHandle. > > // Moving it into WebCore would help make this maintainable and identical across platforms, as well > > // as making it easier to keep in sync with WebCore's client side WebSocket implementation. > > A couple of other reasons I went forward with this WebKit approach were: > > 1. That is what Chromium (outside of WebKit) and Qt (inside WebKit/qt) have done. > 2. I have very little experience with cross platform code. I felt if I could get something > working first for Mac, it would be easier to then move / port that to WebCore, without > originally putting it in WebCore and potentially breaking other ports or superceeding > their implementations. > > Stepping back, #2 could be based on false assumptions I have. If it's important to get this landed soon, it could probably go in in its current form. But if we have a little time to spare I think it's probably worth reworking it. We should talk with the Chromium and Qt people and see if they would be interested in sharing an implementation, if they have any requirements regarding the protocol that we aren't aware of, etc. We should be able to learn from their experience in this area, too. > > 4) It would be nice if we could use WebKit2's CoreIPC layer for handling the IPC. Doing that would require moving CoreIPC out of WebKit2 and into some lower level (which we want to do eventually anyway). > > The only IPC here goes over HTTP requests. I'm not as familiar with WebKit2 as I > would like to be, but I'm not sure this approach would benefit. Does the CoreIPC > layer adapt to network communication between processes, or do you mean that > on the same machine the network could be avoided? Right now this works so that > your browser can inspect a webview on another machine's browser (X) by "downloading" > the Web Inspector frontend (html/css/js) from X and communicating to X via > Web Sockets. CoreIPC right now is all about IPC on a single machine. So I guess you should stick with your current method for now. Comment on attachment 79448 [details]
Part 6: Disallow Remote Inspection on WebView when Private Browsing is Enabled
Marking old patches as obsolete. I will be modifying these patches to make it more generic for other platforms. Basically, it'll put a reusable WebSocket server (compatible with WebCore's WebSocket client code) in WebCore and try to minimize the WebKit code.
I'd like to ask the Qt and Chromium folks how they feel about this, so I'll CC them and ask them.
Jamey and Kenneth, I'm adding you because you did work getting the Remote Inspector up and running for Qt: <http://webkit.org/b/43988> Web Inspector: Remote Web Inspector support for QtWebKit In this bug I'm going to try and move up a similar implementation into WebCore. I plan to design it in a way that should require relatively few changes to move your existing implementation over to this one. I'll try to point out parts platforms would need to implement. If either of you are able, please follow the patches. Your WebKit implementation will still exist, and when this ends I'll open up a bug on Qt to switch over to this version. Pavel, Ilya, Yury, and other Chromium developers are already CC'd. I haven't looked at your implementation, but it probably works similarly. Let me know if the implementation would be suitable for Chromium to use, or if I go down a bad path that would make support difficult for you. Created attachment 79936 [details]
[PATCH] New Part 1 - Generic Server Socket to be used for a WebSocket Server
I call these "New Part" etc. The part 1 and 2 above reviewed by Yurys would also need to land.
Steps that would be needed for a new platform to use this:
1. Add platform/network/ServerSocket* files to their build system.
(NOTE: Should I do this?)
2. Implement a platform specific ServerSocket, which should just be:
- platformListen, platformClose
- calling client callbacks, at least didReceiveConnection (takes a SocketStreamHandle)
3. Probably a new way to create a platform specific SocketStreamHandle
This uses LOG(Network, ...) for Server Socket logs. To enable this in Safari, you
can look up the key for "Network" in Source/WebCore/platform/Logging.cpp,
and it is "0x00100000". So write out the default like so:
shell> defaults write com.apple.safari WebCoreLogLevel 0x00100000
Next up is abstracting + refactoring HTTPRequest from WebSocketRequest
into a generic, reusable, class.
(In reply to comment #33) > Jamey and Kenneth, I'm adding you because you did work getting the Remote Inspector > up and running for Qt: > <http://webkit.org/b/43988> Web Inspector: Remote Web Inspector support for QtWebKit > > In this bug I'm going to try and move up a similar implementation into WebCore. I plan to > design it in a way that should require relatively few changes to move your existing > implementation over to this one. I'll try to point out parts platforms would need to implement. > If either of you are able, please follow the patches. Your WebKit implementation will still > exist, and when this ends I'll open up a bug on Qt to switch over to this version. > > Pavel, Ilya, Yury, and other Chromium developers are already CC'd. I haven't looked at > your implementation, but it probably works similarly. Let me know if the implementation > would be suitable for Chromium to use, or if I go down a bad path that would make > support difficult for you. Nice! sounds pretty good. Thanks for keeping us in form. Created attachment 79986 [details]
[PATCH] New Part 2 - Add Generic HTTP Request Class. Generalized HTTP Parsing.
This part:
• Generalize some HTTP code
• Borrowed code from WebSockets causes WebSockets refactoring
• Should not require any platform specific changes.
Next up is creating a generic WebSocketServer, and either a client
interface or a subclass that would be an "InspectorServer". I'm still
working out the details of what would be best.
Comment on attachment 79936 [details] [PATCH] New Part 1 - Generic Server Socket to be used for a WebSocket Server View in context: https://bugs.webkit.org/attachment.cgi?id=79936&action=review > Source/WebCore/platform/network/cf/SocketStreamHandle.h:63 > + SocketStreamHandle(int nativeFileDescriptor, SocketStreamHandleClient* client); > + > + void createStreams(int nativeFileDescriptor); For network/cf/SocketStreamHandle.h it turns out if I open sockets from a nativeFileDescriptor I should also be sure to manually ::close(fd). The diff is pretty straight forward, I made m_nativeFileDescriptor, no longer need the createStreams() fd argument, and close the fd in platformClose. The user impact is that only a few requests are allowed open at one time, and it looks like the network is stalled. Created attachment 80249 [details]
[PATCH] New Part 1 - Generic Server Socket to be used for a WebSocket Server
Updated this while I had the chance with the change I mentioned above.
Created attachment 80252 [details]
[PATCH] New Part 3 - Generalized WebSocket Frame Parsing and Creating.
No platform specific changes necessary.
Created attachment 80253 [details]
[PATCH] New Part 4 - Add a Generic WebSocket Server.
No platform specific changes necessary.
Created attachment 80254 [details]
[PATCH] New Part 5 - Add InspectorServer logic to handle connections.
This is the last of the WebCore work for a remote inspector server. The rest is up to clients.
To get an idea what a webkit client needs to do see the next patch (Mac Part 1). Namely:
- Implement the InspectorServerClient.h interface in your WebKit/platform
- provide a listing page (html)
- serve inspector resources
- hook up your server client to inspectorServer()->setInspectorClient(...) [such as at startup]
- call inspectorServer()->listen(port) when you want to start listening, and close() when you want to stop
- Implement the InspectorClient additions, and use the remote connection when needed
- setupRemoteConnection
- teardownRemoteConnectionFromRemoteSide
- call inspectorServer()->closeConnection(remoteConnectionId()) when closing
- call inspectorServer()->sendMessageOverConnection(remoteConnectionId(), message) when
sendMessageToFrontend is called and there is a remote connection.
Created attachment 80256 [details]
[PATCH] Mac Part 1: Add Remote Inspector Support
This implements things for mac. I have followup patches, but it might be smarter to move
these to a new Bugzilla bug to not crowd this bug with so much. What do you think?
Comment on attachment 80249 [details] [PATCH] New Part 1 - Generic Server Socket to be used for a WebSocket Server View in context: https://bugs.webkit.org/attachment.cgi?id=80249&action=review > Source/WebCore/ChangeLog:13 > + didReceiveConnection callback with a generic SocketStreamHandle. I'm not sure if "didReceiveConnection" parses. Maybe "establish"? > Source/WebCore/platform/network/ServerSocketHandleBase.cpp:49 > + ASSERT(m_port); > + if (!m_port) > + return; We only do this when fixing super elusive bugs by adding semi-arbitrary null checks. If m_port can't be null, just assert, and don't make runtime checks. > Source/WebCore/platform/network/ServerSocketHandleClient.h:45 > +} // namespace WebCore I'm not a fan of such comments - if you really need to check what this brace at the end of the file is for, you'll double-click it for IDE to balance, and if you're not, why is this boilerplate asking for attention? > Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:58 > + m_serverFileDescriptor = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP); We don't have CF level APIs to create listening sockets? This whole function feels alien, with perror and such. > Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:70 > + sockaddr6.sin6_addr = in6addr_any; Should there be any limit on which interfaces to bind to? Even when running regression tests, we only listen on loopback, because opening up on all interfaces isn't prudent security wise. > Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:157 > + int fd = *(const int*)data; I don't think that const has any real meaning here. Please use a C++ style cast. Comment on attachment 80249 [details] [PATCH] New Part 1 - Generic Server Socket to be used for a WebSocket Server View in context: https://bugs.webkit.org/attachment.cgi?id=80249&action=review >> Source/WebCore/ChangeLog:13 >> + didReceiveConnection callback with a generic SocketStreamHandle. > > I'm not sure if "didReceiveConnection" parses. Maybe "establish"? I use "establish" in later patches on to mean a connection that has had some setup. How about, "didAcceptConnection"? >> Source/WebCore/platform/network/ServerSocketHandleBase.cpp:49 >> + return; > > We only do this when fixing super elusive bugs by adding semi-arbitrary null checks. If m_port can't be null, just assert, and don't make runtime checks. Sounds good. >> Source/WebCore/platform/network/ServerSocketHandleClient.h:45 >> +} // namespace WebCore > > I'm not a fan of such comments - if you really need to check what this brace at the end of the file is for, you'll double-click it for IDE to balance, and if you're not, why is this boilerplate asking for attention? This was just boilerplate I have because all other files have it. I'm fine with removing this and some endif comments. >> Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:58 >> + m_serverFileDescriptor = socket(AF_INET6, SOCK_STREAM, IPPROTO_TCP); > > We don't have CF level APIs to create listening sockets? This whole function feels alien, with perror and such. Correct, I am not aware of any CF level APIs for listening sockets =(. >> Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:70 > > Should there be any limit on which interfaces to bind to? Even when running regression tests, we only listen on loopback, because opening up on all interfaces isn't prudent security wise. Hmm, it was mentioned before that maybe we should only allow localhost and "for cross machine debugging you can use ssh port forwarding etc." I'm just not familiar with how this works. I don't want to leave this as is if there is so much concern. I'll need to investigate this. Comment on attachment 79986 [details] [PATCH] New Part 2 - Add Generic HTTP Request Class. Generalized HTTP Parsing. View in context: https://bugs.webkit.org/attachment.cgi?id=79986&action=review In general, I'm not thrilled to see generic HTTP parsing code in WebCore. Can you use CFHTTPMessage instead? WebSockets got a pass because it's not quite HTTP, and we have some parsing methods where we don't care about precision too much, like in XMLHttpRequest. But using custom HTTP message parsing code for something as subtle as a server (and having it in classes with such generic names) is scary. > Source/WebCore/ChangeLog:31 > + (WebCore::parseHTTPHeaders): taken from previous WebSocket parsing. Algorithm unchanged. This isn't good. WebSocket has all sorts of limitations on how headers can look like - for example, there is no support for continuations. > Source/WebCore/inspector/InspectorInstrumentation.h:264 > - static void willSendWebSocketHandshakeRequestImpl(InspectorController*, unsigned long identifier, const WebSocketHandshakeRequest&); > + static void willSendWebSocketHandshakeRequestImpl(InspectorController*, unsigned long identifier, WebSocketHandshakeRequest*); I don't understand these changes. Can the argument be null? > Source/WebCore/platform/network/HTTPParseError.h:43 > + HTTPParseError(const String& description) > + : description(description) > + { > + } This should initialize wasLikelyIncompleteInput. > Source/WebCore/platform/network/HTTPParsers.cpp:414 > + error.description = "Incomplete Request Line"; WebCore calls out to WebKit for error descriptions, because they generally need to be localized. > Source/WebCore/platform/network/HTTPParsers.cpp:513 > + AtomicString nameStr(String::fromUTF8(name.data(), name.size())); > + String valueStr = String::fromUTF8(value.data(), value.size()); HTTP headers are not required to be UTF-8, and frequently aren't. > Source/WebCore/platform/network/HTTPParsers.cpp:524 > + // FIXME: should not be log error, should be some other type of logging. > + LOG_ERROR("name=%s value=%s", nameStr.string().utf8().data(), valueStr.utf8().data()); Did you intend to address this before submitting? > Source/WebCore/platform/network/HTTPRequest.cpp:92 > + : m_url(KURL()) This isn't needed. > Source/WebCore/platform/network/HTTPRequest.cpp:94 > + , m_requestMethod(String()) Neither is this. > Source/WebCore/websockets/WebSocketHandshake.cpp:500 > + HTTPParseError error = HTTPParseError(); HTTPParseError has a default constructor, no need to initialize it like a struct. Re part 3 and 4 - implementing a version 76 WebSocket server in WebCore isn't great because this spec is obsolete. A lot of this code is going to be rewritten, and having a C++ server side to take care of would probably complicate things a lot. Not sure what you could do here though. (In reply to comment #45) > (From update of attachment 79986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79986&action=review > > In general, I'm not thrilled to see generic HTTP parsing code in WebCore. Can you use CFHTTPMessage instead? > > WebSockets got a pass because it's not quite HTTP, and we have some parsing methods where we don't care about precision too much, like in XMLHttpRequest. But using custom HTTP message parsing code for something as subtle as a server (and having it in classes with such generic names) is scary. > > Source/WebCore/ChangeLog:31 > > + (WebCore::parseHTTPHeaders): taken from previous WebSocket parsing. Algorithm unchanged. > > This isn't good. WebSocket has all sorts of limitations on how headers can look like - for example, there is no support for continuations. > > Source/WebCore/platform/network/HTTPParsers.cpp:513 > > + AtomicString nameStr(String::fromUTF8(name.data(), name.size())); > > + String valueStr = String::fromUTF8(value.data(), value.size()); > > HTTP headers are not required to be UTF-8, and frequently aren't. I used CFHTTPMessages in obsoleted patches. I guess I could make yet another platform facade for handling HTTP Messages, but I don't know how I would be able to share that in WebCore. Would you be happier with this if the files were renamed to be less generic, and maybe all kept inside of WebCore/websockets? > > Source/WebCore/inspector/InspectorInstrumentation.h:264 > > - static void willSendWebSocketHandshakeRequestImpl(InspectorController*, unsigned long identifier, const WebSocketHandshakeRequest&); > > + static void willSendWebSocketHandshakeRequestImpl(InspectorController*, unsigned long identifier, WebSocketHandshakeRequest*); > > I don't understand these changes. Can the argument be null? In this patch the class WebSocketHandshakeRequest became a subclass of RefCounted HTTPRequest, and so I found it easier to pass a pointer to the request (refCountedRequest.get()). Is there a convenient way I could stick with the old prototype? > > Source/WebCore/platform/network/HTTPParseError.h:43 > > + HTTPParseError(const String& description) > > + : description(description) > > + { > > + } > > This should initialize wasLikelyIncompleteInput. Good catch. > > Source/WebCore/platform/network/HTTPParsers.cpp:414 > > + error.description = "Incomplete Request Line"; > > WebCore calls out to WebKit for error descriptions, because they generally need to be localized. This is unchanged from the existing patch. But yes, it's worth a new bug. <http://webkit.org/b/53224> Web Socket Console Messages Are Not Localized > > > Source/WebCore/platform/network/HTTPParsers.cpp:524 > > + // FIXME: should not be log error, should be some other type of logging. > > + LOG_ERROR("name=%s value=%s", nameStr.string().utf8().data(), valueStr.utf8().data()); > > Did you intend to address this before submitting? Yes, I should probably go with LOG(Network, ...) or removed it entirely. > > Source/WebCore/platform/network/HTTPRequest.cpp:92 > > + : m_url(KURL()) > > This isn't needed. > > > Source/WebCore/platform/network/HTTPRequest.cpp:94 > > + , m_requestMethod(String()) > > Neither is this. > > > Source/WebCore/websockets/WebSocketHandshake.cpp:500 > > + HTTPParseError error = HTTPParseError(); > > HTTPParseError has a default constructor, no need to initialize it like a struct. Sounds good for each of these. (In reply to comment #46) > Re part 3 and 4 - implementing a version 76 WebSocket server in WebCore isn't great > because this spec is obsolete. A lot of this code is going to be rewritten, and having a > C++ server side to take care of would probably complicate things a lot. Not sure what > you could do here though. The goal of this was to have a WebSocket server that works with the WebSocket client implementation in WebCore. Having the source of the two sides sharing much of the same code in WebCore/websockets (like WebSocketFraming.{h,cpp}) should limit much of the rewriting. Given all of your feedback, how do you feel about the obsoleted patch I put up earlier? It was up for weeks despite my poking for review comments. It was Mac only, and therefore makes use of CFHTTPRequest, and CFRead/Write streams more directly. I think an approach like this would actually be harder to keep up to date with WebCore's WebSocket client implementation: https://bugs.webkit.org/attachment.cgi?id=77066&action=review (In reply to comment #48) > (In reply to comment #46) > > Re part 3 and 4 - implementing a version 76 WebSocket server in WebCore isn't great > > because this spec is obsolete. A lot of this code is going to be rewritten, and having a > > C++ server side to take care of would probably complicate things a lot. Not sure what > > you could do here though. By the way, to this end I was able to locally create some boilerplate and eventually a LayoutTest that would sanity check that a WebSocket can open, send, receive, and close a connection with a WebSocketServer as defined here. My changes for the test were ugly, as I made an Obj-C WebSocketServer class to use in DumpRenderTree, but such a test might be useful to ensure the client / server don't get out of sync. Currently it looks like `run-new-webkit-websocketserver` uses pywebsocket. What does that mean for when we update our WebSocket client implementation to a new draft or edition? Do we also have to update pywebsocket? Wait for a new version? Find a new server library to test against? Or would it be easier to maintain our own server, as above? Comment on attachment 80254 [details] [PATCH] New Part 5 - Add InspectorServer logic to handle connections. View in context: https://bugs.webkit.org/attachment.cgi?id=80254&action=review > Source/WebCore/inspector/InspectorServer.cpp:1 > +/* My biggest concern is that this class lives in the WebCore. In Chromium, we can't do page discovery from within the WebCore and we also can't open listen socket from there. What is it going to be like in WebKit2? I was thinking that we implement it a bit differently: - WebCore (or WebKit/cf) contains code that can be reused by the platforms in order to support HTTP/WebSocket server infrastructure - WebKit layer instantiates it and pipes messages to the corresponding InspectorController instance. The code looks nice and clean btw, great job! > Would you be happier with this if the files were renamed to be less generic, and maybe all > kept inside of WebCore/websockets? Probably. If it's only for WebSockets-76, we shouldn't pretend it's HTTP. But I'm not sure about future versions - I've heard that those are fully HTTP compatible, so having a full cross-platform implementation could be less than great. > In this patch the class WebSocketHandshakeRequest became a subclass of RefCounted HTTPRequest, > and so I found it easier to pass a pointer to the request (refCountedRequest.get()). Is there a convenient > way I could stick with the old prototype? It's not a very common idiom in WebCore, but I like to pass by reference when arguments can't be null. You can just do *refCountedRequest.get(). > This is unchanged from the existing patch. But yes, it's worth a new bug. > <http://webkit.org/b/53224> Web Socket Console Messages Are Not Localized I don't know if other console messages are localized. > Currently it looks like `run-new-webkit-websocketserver` uses pywebsocket. What does > that mean for when we update our WebSocket client implementation to a new draft or > edition? Do we also have to update pywebsocket? Wait for a new version? Find a new > server library to test against? Or would it be easier to maintain our own server, as above? Although pywebsocket is a separate project, it's being developed together with WebCore WebSocket implementation. (In reply to comment #51) > > In this patch the class WebSocketHandshakeRequest became a subclass of RefCounted HTTPRequest, > > and so I found it easier to pass a pointer to the request (refCountedRequest.get()). Is there a convenient > > way I could stick with the old prototype? > > It's not a very common idiom in WebCore, but I like to pass by reference when arguments can't be null. You can just do *refCountedRequest.get(). Or even just *refCountedRequest. (In reply to comment #50) > (From update of attachment 80254 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80254&action=review > > > Source/WebCore/inspector/InspectorServer.cpp:1 > > +/* > > My biggest concern is that this class lives in the WebCore. In Chromium, we can't do page discovery from within the WebCore and we also can't open listen socket from there. The main advantage of having InspectorServer be in WebCore was to share some of the code for setting up connections and disallowing remote inspection. For example, the upgrade request paths "/devtools/page/<num>", listing page "/", disallow for private browsing, existing inspectors, etc. But that isn't too much code, and maybe I could come up with a way to share it. Suggestions are appreciated. > What is it going to be like in WebKit2? I was thinking that we implement it a bit differently: > - WebCore (or WebKit/cf) contains code that can be reused by the platforms in order to support HTTP/WebSocket server infrastructure > - WebKit layer instantiates it and pipes messages to the corresponding InspectorController instance. Unfortunately I don't know much about the cross platform setups (Chromium and WebKit2). It sounds like to ensure there really is only 1 InspectorServer it should be in the WebKit layer. As for WebKit2, I'd be interested how other singletons, like the PageCache work. (In reply to comment #53) > (In reply to comment #50) > > (From update of attachment 80254 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=80254&action=review > > > > > Source/WebCore/inspector/InspectorServer.cpp:1 > > > +/* > > > > My biggest concern is that this class lives in the WebCore. In Chromium, we can't do page discovery from within the WebCore and we also can't open listen socket from there. > > The main advantage of having InspectorServer be in WebCore was to share some of the > code for setting up connections and disallowing remote inspection. For example, the > upgrade request paths "/devtools/page/<num>", listing page "/", disallow for private > browsing, existing inspectors, etc. But that isn't too much code, and maybe I could > come up with a way to share it. Suggestions are appreciated. > Not sure I follow. But the way I understand it, I disagree. I think that page discovery is very browser-specific. Embedded devices that have single tab will do it their way, multi-tab browsers, the other. We don't have to standardize upon /devtools/page/<num> as long as discovery schema is shared. Furthermore, I am not sure we need to share it up front. My point here is that the less constraints we add, the wider protocol adoption is. > > What is it going to be like in WebKit2? I was thinking that we implement it a bit differently: > > - WebCore (or WebKit/cf) contains code that can be reused by the platforms in order to support HTTP/WebSocket server infrastructure > > - WebKit layer instantiates it and pipes messages to the corresponding InspectorController instance. > > Unfortunately I don't know much about the cross platform setups (Chromium and WebKit2). > It sounds like to ensure there really is only 1 InspectorServer it should be in the WebKit layer. > As for WebKit2, I'd be interested how other singletons, like the PageCache work. I can tell you how I am seeing it: Pre-upgrade: - HTTP server socket implementation lives in the browser (at least in WebKit) - Static files are served by the WebKit in a platform-specific, tab-agnostic manner. Only WebKit knows where the files are located. Mobile device can even redirect to a web-hosted version of matching front-end version. Post-upgdate: - WebKit exposes thin remote debugging API that wraps InspectorController's API. The latest vision is that InspectorController exposes API that is only used by WebKit (further exposed by WebKit to its clients). InspectorController's API soon will be connect, disconnect and dispatchMessageFromFrontend. Only tab-specific messages are reaching WebCore. They are already cracked payloads of the transport messages. Such a layered structure allows us to be non-websocket bound (the standard is changing again). Imagine serving InspectorBackend that would use long GET instead of websocket (or any other standard). It also leaves a lot of flexibility for the platform owners. Now the question is how we could make your nice WebSocket code reused between platform vendors and how much of the InspectorServer goodness we would like to reuse. > > Source/WebCore/platform/network/cf/ServerSocketHandleCFNet.cpp:157
> > + int fd = *(const int*)data;
>
> I don't think that const has any real meaning here.
FWIW I get a build issue otherwise:
static_cast from type 'const void*' to type 'int*' casts away constness
> static_cast from type 'const void*' to type 'int*' casts away constness
Yes, you need to use both static_cast and const_cast. We still prefer that over C-style cast.
Um, I mean theat we would use that if we needed an int*, which we don't need here. Just *static_cast<const int*>(data) would do. Comment on attachment 77064 [details]
[PATCH] Part 1: Change Code Generation for InspectorBackendDispatcher
Original part 1 no longer needed with the only InspectorBackendDispatcher method call no longer needed to be exported thanks to InspectorController::dispatchMessageFromFrontend.
Comment on attachment 77065 [details] [PATCH] Part 2: Update Styles for when the Inspector Web Page is Used Remotely Original Part 2 landed in: <http://trac.webkit.org/changeset/84023> Can we split this off into multiple bugs? This bug hasn't been touched in over a month and it's unclear what if anything needs to be done here. Comment on attachment 80256 [details]
[PATCH] Mac Part 1: Add Remote Inspector Support
Removed review flags. Some of this is already out of date due
to changes in the last few months.
(In reply to comment #60) > Can we split this off into multiple bugs? This bug hasn't been touched in over > a month and it's unclear what if anything needs to be done here. If this gets readdressed in the future it can be split into multiple bugs. Most of the earlier patches would still be valid, but its useless landing them without them being used by the later patches. They would just be dead code. To clarify what could be done, Pavel raised some concerns that in a multiprocess model page discovery cannot be handled in WebCore like "New Part 5" tried to do. That is a valid concern for Chromium and probably WebKit2 as well. So, handling page discovery could be moved into the WebKit client layer. Here was his comment: (In reply to comment #50) > (From update of attachment 80254 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=80254&action=review > > My biggest concern is that this class lives in the WebCore. In Chromium, we can't do > page discovery from within the WebCore and we also can't open listen socket from > there. What is it going to be like in WebKit2? I was thinking that we implement it a bit differently: > - WebCore (or WebKit/cf) contains code that can be reused by the platforms in order to support HTTP/WebSocket server infrastructure > - WebKit layer instantiates it and pipes messages to the corresponding InspectorController instance. Comment on attachment 77064 [details] [PATCH] Part 1: Change Code Generation for InspectorBackendDispatcher Cleared Yury Semikhatsky's review+ from obsolete attachment 77064 [details] so that this bug does not appear in http://webkit.org/pending-commit. Comment on attachment 77065 [details] [PATCH] Part 2: Update Styles for when the Inspector Web Page is Used Remotely Cleared Yury Semikhatsky's review+ from obsolete attachment 77065 [details] so that this bug does not appear in http://webkit.org/pending-commit. I've taken a look at those patches and did something for the Qt port on WebKit2. We are planning to stick only with a cross-host remote inspector support for our first WebKit2 release. The server has to be enabled through an environment variable for the Qt port, by default bound on localhost but configurable to other interfaces. Here is the way I saw the layer separation: - WebCore contains the WebSocket server responsible for handling HTTP and WebSocket streams. - The WebKit2 UI process starts the server and handles the registration of pages that want to be inspected. It forwards connection notifications and messages from the front-end to WebInspectorProxy which sends them to the InspectorController through IPC. - All HTTP requests for resources and the index pages are handed to the platform implementation so that ports can have different page listings and storage for the front-end. Most of the code is the same. What I've done is rebase them, strip out all the Mac code paths, implement the WebKit2/Qt part, address the concerns in comments here and fix nitpicks here and there. They were uploaded to different bugs, tell me what you think. Comment on attachment 80249 [details] [PATCH] New Part 1 - Generic Server Socket to be used for a WebSocket Server Now tracked in bug #73090. Comment on attachment 79986 [details]
[PATCH] New Part 2 - Add Generic HTTP Request Class. Generalized HTTP Parsing.
Now tracked in but #73092.
Comment on attachment 80252 [details] [PATCH] New Part 3 - Generalized WebSocket Frame Parsing and Creating. Now tracked in bug #73093 (along with Part 4). Comment on attachment 80253 [details] [PATCH] New Part 4 - Add a Generic WebSocket Server. Now tracked in bug #73093. Comment on attachment 80254 [details] [PATCH] New Part 5 - Add InspectorServer logic to handle connections. Now tracked in bug #73094. > Most of the code is the same. What I've done is rebase them, strip out all the Mac code paths, implement the WebKit2/Qt part, address the concerns in comments here and fix nitpicks here and there.
> They were uploaded to different bugs, tell me what you think.
Your approach looks good. I'd like to assess what it takes to make chromium use this upstream implementation and whether it is flexible enough.
|