| Summary: | [WebSocket] Must ignore incoming message in CLOSING state | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Takashi Toyoshima <toyoshim> | ||||||||||||||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Takashi Toyoshima <toyoshim> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | ap, bashi, buildbot, commit-queue, dglazkov, lamarque, rniwa, tkent, webkit.review.bot, yutak | ||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Takashi Toyoshima
2012-05-08 17:34:46 PDT
Created attachment 195363 [details]
Patch
Proposed patch
Comment on attachment 195363 [details] Patch Attachment 195363 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17314421 New failing tests: http/tests/websocket/tests/hybi/client-close.html Created attachment 195380 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Created attachment 195383 [details]
Patch
Rebaseline client-close.html because of change in the description field.
Comment on attachment 195383 [details] Patch Attachment 195383 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17312553 New failing tests: http/tests/websocket/tests/hybi/client-close.html Created attachment 195402 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2
Comment on attachment 195383 [details] Patch Attachment 195383 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17313356 Created attachment 195650 [details]
Archive of layout-test-results from webkit-ews-07 for mac-future
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-future Platform: Mac OS X 10.8.2
Comment on attachment 195383 [details] Patch Attachment 195383 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17309556 New failing tests: http/tests/websocket/tests/hybi/client-close.html Created attachment 195657 [details]
Archive of layout-test-results from webkit-ews-03 for mac-future
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03 Port: mac-future Platform: Mac OS X 10.8.2
Created attachment 195693 [details]
Patch
Split close-client.html into two unit tests.
Comment on attachment 195693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195693&action=review Hi Lamarque, Thank you for sending a patch to fix this. Actually, I'm not an authoritative WebKit reviewer, but I'd say some comments on this patch as a WebKit committer working on WebSocket. > LayoutTests/http/tests/websocket/tests/hybi/client-close-ignore-incoming.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Old tests still uses this document type, but we adopt HTML5 doctype <!DOCTYPE html> for new tests. It's better to fix other files which you touched in this change. > LayoutTests/http/tests/websocket/tests/hybi/client-close-ignore-incoming.html:4 > +<script src="../../../../js-test-resources/js-test-pre.js"></script> For the same reason, please don't use relative path, but absolute path here and the end of this html. You may want to see useragent-in-openinghandshake.html as a test which is introduced recently. > LayoutTests/http/tests/websocket/tests/hybi/client-close-ignore-incoming.html:10 > +description("WebSocket: Test client-initiated close. After WebSocket.close() any message from server must be discarded by WebSocket stack according to the new WebSocket API, see http://webkit.org/b/85934."); I think it is not usual to describe webkit bug here. If you want to say something on this test, please write them in ChangeLog. > LayoutTests/http/tests/websocket/tests/hybi/client-close_wsh.py:14 > This test intends to check a client sending close frame itself. So, this change make it meaningless. Please read comments in this test carefully. Comment on attachment 195693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195693&action=review >> LayoutTests/http/tests/websocket/tests/hybi/client-close-ignore-incoming.html:10 >> +description("WebSocket: Test client-initiated close. After WebSocket.close() any message from server must be discarded by WebSocket stack according to the new WebSocket API, see http://webkit.org/b/85934."); > > I think it is not usual to describe webkit bug here. > If you want to say something on this test, please write them in ChangeLog. Well, I have added the link to the bug that motivated changes before and the other reviewers did not complain about it. The description also says what is the expected behavior for this unit test. In this particular case it is important because we expect a message to be ignored. >> LayoutTests/http/tests/websocket/tests/hybi/client-close_wsh.py:14 >> > > This test intends to check a client sending close frame itself. So, this change make it meaningless. > Please read comments in this test carefully. Yeah, I was in doubt if this change was correct. I think this unit test can be discarded then. After this change there is no way to make the line "msgutil.send_message(request, 'close_frame[:2]=%r' % close_frame[:2])" work since the client will ignore the message. The new unit test I created (client-close-ignore-incomming.html) covers the new behavior. I am also worried about this change. A lot of websocket scripts out there will have to be updated to avoid the same regressions I have found in our unit tests. Created attachment 196245 [details]
Patch
Fix issues reported.
Comment on attachment 196245 [details] Patch Attachment 196245 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17366537 New failing tests: http/tests/websocket/tests/hybi/client-close.html Created attachment 196255 [details]
Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 195693 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195693&action=review >>> LayoutTests/http/tests/websocket/tests/hybi/client-close_wsh.py:14 >>> >> >> This test intends to check a client sending close frame itself. So, this change make it meaningless. >> Please read comments in this test carefully. > > Yeah, I was in doubt if this change was correct. I think this unit test can be discarded then. After this change there is no way to make the line "msgutil.send_message(request, 'close_frame[:2]=%r' % close_frame[:2])" work since the client will ignore the message. The new unit test I created (client-close-ignore-incomming.html) covers the new behavior. I am also worried about this change. A lot of websocket scripts out there will have to be updated to avoid the same regressions I have found in our unit tests. How about sending a close frame response iff close_frame[:2] is expected data. You can check CloseEvent.wasClean to know if the close_frame contains expected data. Or you can embed arbitrary data to reason field of close frame which can be read as CloseEvent.reason in browser. See also close-code-and-reason_wsh.py. Created attachment 196687 [details]
Patch
Use Takashi's suggestion to send message embedded in close frame.
(In reply to comment #17) > How about sending a close frame response iff close_frame[:2] is expected data. You can check CloseEvent.wasClean to know if the close_frame contains expected data. > Or you can embed arbitrary data to reason field of close frame which can be read as CloseEvent.reason in browser. > See also close-code-and-reason_wsh.py. It works. Thanks for the tip :-) Cool. Thanks! This change looks good. Please ask ap@webkit.org for the final review to commit this. After that, I'll try to push this change to Blink, too. Comment on attachment 196687 [details]
Patch
ok
The commit-queue encountered the following flaky tests while processing attachment 196687 [details]: svg/batik/paints/patternPreserveAspectRatioA.svg bug 114139 (author: zimmermann@kde.org) svg/as-image/img-relative-height.html bug 114140 (author: zimmermann@kde.org) The commit-queue is continuing to process your patch. Comment on attachment 196687 [details] Patch Rejecting attachment 196687 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 196687, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: fs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 147890 = dce53a8d75be88d4b224a35bcb48af24022fccf2 r147891 = d4d91ae23cf49fdb88e5502039deeff04627efeb r147892 = 4e3f96534501c6afed9a27a90e39f5df36b51763 r147893 = 11e52cf9f60d507c230704947ae0c2db8e17da9b Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-commit-queue.appspot.com/results/17562005 Comment on attachment 196687 [details]
Patch
Seems cq bot had some issues, let us try again...
Comment on attachment 196687 [details] Patch Rejecting attachment 196687 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 196687, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 331ac3fe M LayoutTests :040000 040000 24d9b3d66502bb5ba11668a063affddcac162f42 9f1b470b1b5cf49471fd367583018bd6eb6da966 M Source Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-commit-queue.appspot.com/results/17491111 Comment on attachment 196687 [details]
Patch
Please re-upload the patch with webkit-patch upload. git diff wouldn't apply.
Created attachment 196881 [details]
Patch
Uploading patch again for landing.
Comment on attachment 196881 [details] Patch Rejecting attachment 196881 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 196881, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: /git.webkit.org/WebKit 14ddb34..da4c535 master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 147946 = 14ddb34f2dd43fc51a356679ea878cc05dc02c9c r147947 = da4c535f8abc55a094169db9f19f6c67fd8c89c7 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-commit-queue.appspot.com/results/17575044 Created attachment 196966 [details]
Patch
Uploading patch for landing. I ran git-fsck on my tree it fixed several dangling commits, let's see if it works now.
Comment on attachment 196966 [details] Patch Rejecting attachment 196966 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 196966, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 495ba2d4a83c234980e8d72e9f1e90440e8 r147964 = 1a247a3ac5dc5f789ee359644edb1c415508ca63 r147965 = ee10e45215b3cbe9629cd0c679f26503590ff3c1 r147966 = 924692f6afc55819809ce99ddbeb7d8758b0caba r147967 = 4ca681abae66e2ce349a6589c55e230212cf5fb4 r147968 = cf28c9c62e25be7b1e23fd9e53550278c6c7b520 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://webkit-commit-queue.appspot.com/results/17636010 I guess you created this branch from a local trunk which has wrong hash value because of unexpected local changes. Can you take a clean checkout and create a new branch. Then apply this patch to it. After that, webkit-patch will work hopefully. (In reply to comment #31) > I guess you created this branch from a local trunk which has wrong hash value because of unexpected local changes. > Can you take a clean checkout and create a new branch. Then apply this patch to it. After that, webkit-patch will work hopefully. Hi, I have never created a branch in my tree and I use git, not subversion. A clean checkout is going to take two or three days due to my slow Internet connection :-/ Usually we create a local branch for each bug, then make changes in the branch. If you work at a local trunk, please revert all local commits firstly. After that, make it up to date by synching with remote trunk. It must be equivalent of a clean checkout. Created attachment 196985 [details]
Patch
Uploading patch for landing.
Comment on attachment 196985 [details] Patch Rejecting attachment 196985 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 196985, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 8bae380b M LayoutTests :040000 040000 159beb0a331e0fec27542bd290529f911c6ffcdc 772fa7104dcdff27ec119070e3be30443252aca6 M Source Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Full output: http://webkit-commit-queue.appspot.com/results/17696014 Comment on attachment 196985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196985&action=review > LayoutTests/ChangeLog:7 > + Need a short description (OOPS!). > + Need the bug URL (OOPS!). Please remove these lines. Created attachment 197034 [details]
Patch
Fixed problem in LayoutTests/ChangeLog.
The commit-queue encountered the following flaky tests while processing attachment 197034 [details]: svg/dom/SVGScriptElement/script-load-and-error-events.svg bug 114279 (authors: fmalita@chromium.org, yurys@chromium.org, and zimmermann@kde.org) svg/animations/smil-leak-elements.svg bug 114280 (authors: fmalita@chromium.org and timothy_horton@apple.com) svg/animations/smil-leak-dynamically-added-element-instances.svg bug 114281 (authors: fmalita@chromium.org and timothy_horton@apple.com) transitions/color-transition-rounding.html bug 114182 (author: simon.fraser@apple.com) transitions/cubic-bezier-overflow-svg-length.html bug 114183 (author: peter@chromium.org) transitions/interrupt-zero-duration.html bug 114184 (authors: cmarrin@apple.com, rniwa@webkit.org, and simon.fraser@apple.com) transitions/multiple-background-transitions.html bug 114185 (author: simon.fraser@apple.com) transitions/cubic-bezier-overflow-color.html bug 114186 (author: peter@chromium.org) transitions/multiple-shadow-transitions.html bug 114187 (author: simon.fraser@apple.com) transitions/mismatched-shadow-transitions.html bug 114188 (author: simon.fraser@apple.com) transitions/color-transition-all.html bug 114189 (authors: ossy@webkit.org and simon.fraser@apple.com) transitions/cubic-bezier-overflow-shadow.html bug 114191 (author: peter@chromium.org) transitions/min-max-width-height-transitions.html bug 114192 (author: simon.fraser@apple.com) transitions/cancel-transition.html bug 114193 (authors: ojan@chromium.org, rniwa@webkit.org, and simon.fraser@apple.com) transitions/border-radius-transition.html bug 114194 (author: simon.fraser@apple.com) transitions/flex-transitions.html bug 114195 (author: tony@chromium.org) transitions/mixed-type.html bug 114196 (author: mikelawther@chromium.org) transitions/multiple-mask-transitions.html bug 114197 (author: simon.fraser@apple.com) transitions/color-transition-premultiplied.html bug 114198 (author: simon.fraser@apple.com) transitions/mismatched-shadow-styles.html bug 114199 (author: simon.fraser@apple.com) transitions/mask-transitions.html bug 114200 (authors: ojan@chromium.org, oliver@apple.com, and simon.fraser@apple.com) transitions/cubic-bezier-overflow-length.html bug 114201 (author: peter@chromium.org) transitions/multiple-background-size-transitions.html bug 114202 (authors: mitz@webkit.org and simon.fraser@apple.com) transitions/clip-transition.html bug 114203 (authors: dglazkov@chromium.org and simon.fraser@apple.com) transitions/cubic-bezier-overflow-transform.html bug 114204 (author: peter@chromium.org) transitions/interrupted-accelerated-transition.html bug 56242 (authors: rniwa@webkit.org, simon.fraser@apple.com, and tonyg@chromium.org) transitions/background-transitions.html bug 114206 (author: simon.fraser@apple.com) http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html bug 114208 (authors: abarth@webkit.org and rniwa@webkit.org) fast/loader/javascript-url-in-object.html bug 114210 (authors: rniwa@webkit.org and sam@webkit.org) platform/mac/editing/deleting/deletionUI-single-instance.html bug 114181 (author: rniwa@webkit.org) The commit-queue is continuing to process your patch. Comment on attachment 197034 [details] Patch Clearing flags on attachment: 197034 Committed r148019: <http://trac.webkit.org/changeset/148019> (In reply to comment #36) > (From update of attachment 196985 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196985&action=review > > > LayoutTests/ChangeLog:7 > > + Need a short description (OOPS!). > > + Need the bug URL (OOPS!). > > Please remove these lines. Thanks for the tip. I am wondering why the style bot did not catch that problem. This got landed in <http://trac.webkit.org/changeset/148019>. Commit queue didn't mark the bug resolved because there was an older patch with r+ that wasn't obsolete. |