21939 2008-10-28 16:44:29 -0700 Uninitialized ExceptionCode in DOMWindow::postMessage 2009-03-03 23:20:28 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 mbelshe mbelshe ap darin oldest_to_newest 96937 0 mbelshe 2008-10-28 16:44:29 -0700 Several functions in MessagePort don't initialize their ExceptionCodes. This caused me trouble in DOMWindow::postMessage, which calls MessagePort::clone. The call succeeded, but the uninitialized ec value caused a failure. 96938 1 24736 mbelshe 2008-10-28 16:45:58 -0700 Created attachment 24736 Initialize the exceptioncodes. 96939 2 24737 mbelshe 2008-10-28 16:50:06 -0700 Created attachment 24737 Updated patch. Same patch as before, but this one contains a changelog. 96940 3 24737 darin 2008-10-28 16:54:27 -0700 Comment on attachment 24737 Updated patch. The rules for ExceptionCode& arguments are: 1) If there's no exception, the function has no obligation to set the exception code. 2) If a caller wants to look at the exception code, it's responsible for setting it to 0 before calling. So there's no bug here. 96967 4 ap 2008-10-29 03:50:45 -0700 And the caller (JSDOMWindow::postMessage()) does initialize ec - maybe the issue you are seeing is V8-specific? 96982 5 mbelshe 2008-10-29 08:45:18 -0700 Thanks Darin for clarification on the convention; I didn't know that. Given that, the changes to MessagePort.cpp are unnecessary. Part of this patch is still valid, I think: DOMWindow.cpp DOMWindow::postMessage calls messagePort->clone with an uninitialized ec. Re-marking for review. 96983 6 24737 mbelshe 2008-10-29 08:45:55 -0700 Comment on attachment 24737 Updated patch. See my comments about the changes to DOMWindow.cpp, which I think is still a valid bug. 96984 7 mbelshe 2008-10-29 08:52:54 -0700 OK - nevermind. Alexey - I see what you are saying. We want the declarer to initialize. 96985 8 mbelshe 2008-10-29 09:03:54 -0700 I might have closed this prematurely. As per Darin, we want the declarer of the ExceptionCode to initialize. That makes sense, and that is why I closed this bug. However, DOMWindow::postMessage is non-deterministic based on whether the caller initialized the ExceptionCode before calling. DOMWindow::postMessage uses ec internally with messagePort->clone(). If the caller of postMessage didn't initialize it, postMessage will have undefined behavior. (clone won't touch ec unless there is an exception, so when we come out of clone() postMessage will be using an uninitialized ec). There are two possible fixes; either we modify postMessage to use a different ExceptionCode for calling into clone() or we initialize ec within postMessage as per the patch here. Does this sound right? 96988 9 darin 2008-10-29 09:20:23 -0700 (In reply to comment #8) > DOMWindow::postMessage uses ec internally with messagePort->clone(). If the > caller of postMessage didn't initialize it, postMessage will have undefined > behavior. (clone won't touch ec unless there is an exception, so when we come > out of clone() postMessage will be using an uninitialized ec). > > There are two possible fixes; either we modify postMessage to use a different > ExceptionCode for calling into clone() or we initialize ec within postMessage > as per the patch here. > > Does this sound right? Yes. Retitled bug to reflect this problem. 97002 10 ap 2008-10-29 11:05:00 -0700 Hmm - still doesn't make sense to me. JS bindings must initialize ec before calling DOMWindow::postMessage(), so of course its behavior is undefined if that didn't happen. What am I missing? 97008 11 ap 2008-10-29 11:24:02 -0700 Or are you saying that some other code could raise an exception before DOMWindow::postMessage (something that doesn't currently happen, as it's only JS bindings that call it)? Looking at some existing code, I see ec being cleared before making a call in DOM, but this seems really weird, as this loses information about the earlier exception. 97023 12 darin 2008-10-29 13:06:03 -0700 (In reply to comment #10) > JS bindings must initialize ec before > calling DOMWindow::postMessage(), so of course its behavior is undefined if > that didn't happen. What am I missing? Yes, JavaScript bindings will always initialize ec. But other callers within WebCore are allowed to call the function without initializing ec if they wish to make the call and ignore the exception code. That's our current rule for the DOM. We could change it. A quick grep found at least 31 call sites that make calls and pass uninitialized ExceptionCode arguments because the caller didn't plan to look at the result. For examples of this, look at the call to Node::remove() inside Node::normalize(). And the call to EventTargetNode::dispatchEvent() inside EventHandler::keyEvent(). 97024 13 darin 2008-10-29 13:07:27 -0700 (In reply to comment #11) > Looking at some existing code, I see ec being > cleared before making a call in DOM, but this seems really weird, as this loses > information about the earlier exception. Generally speaking, if there is an earlier exception, it's the obligation of the implementer to return at that point and not try to do further work. It may be easier to discuss specific cases. 97034 14 ap 2008-10-29 14:01:51 -0700 I see, thank you for the clarification! 112040 15 28229 mbelshe 2009-03-03 10:48:20 -0800 Created attachment 28229 revised patch Finally cycled back to this bug; revised the patch just to contain the one ec initialization which we agreed was important. 112138 16 28229 ap 2009-03-03 22:46:46 -0800 Comment on attachment 28229 revised patch r=me (I hope Darin doesn't mind!) Please be sure to add bug URL and title to ChangeLog when landing. 112139 17 fishd 2009-03-03 23:20:28 -0800 Landed as http://trac.webkit.org/changeset/41418 24736 2008-10-28 16:45:58 -0700 2008-10-28 16:50:29 -0700 Initialize the exceptioncodes. MessagePort.ecinit.patch text/plain 1561 mbelshe SW5kZXg6IFdlYkNvcmUvZG9tL01lc3NhZ2VQb3J0LmNwcA0KPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIFdlYkNv cmUvZG9tL01lc3NhZ2VQb3J0LmNwcAkocmV2aXNpb24gMzc5NDgpCisrKyBXZWJDb3JlL2RvbS9N ZXNzYWdlUG9ydC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTk1LDYgKzk1LDggQEAgUGFzc1JlZlB0 cjxNZXNzYWdlUG9ydD4gTWVzc2FnZVBvcnQ6OmNsbwogICAgICAgICByZXR1cm4gMDsKICAgICB9 CiAKKyAgICBlYyA9IDA7CisKICAgICBSZWZQdHI8TWVzc2FnZVBvcnQ+IHJlbW90ZVBvcnQgPSBt X2VudGFuZ2xlZFBvcnQ7CiAgICAgUmVmUHRyPE1lc3NhZ2VQb3J0PiBuZXdQb3J0ID0gTWVzc2Fn ZVBvcnQ6OmNyZWF0ZShuZXdPd25lcik7CiAKQEAgLTExNSw2ICsxMTcsOCBAQCB2b2lkIE1lc3Nh Z2VQb3J0Ojpwb3N0TWVzc2FnZShjb25zdCBTdHJpCiAKIHZvaWQgTWVzc2FnZVBvcnQ6OnBvc3RN ZXNzYWdlKGNvbnN0IFN0cmluZyYgbWVzc2FnZSwgTWVzc2FnZVBvcnQqIGRhdGFQb3J0LCBFeGNl cHRpb25Db2RlJiBlYykKIHsKKyAgICBlYyA9IDA7CisKICAgICBpZiAoIW1fZW50YW5nbGVkUG9y dCB8fCAhbV9kb2N1bWVudCB8fCAhbV9lbnRhbmdsZWRQb3J0LT5tX2RvY3VtZW50KQogICAgICAg ICByZXR1cm47CiAKQEAgLTI5Myw2ICsyOTcsOCBAQCBib29sIE1lc3NhZ2VQb3J0OjpkaXNwYXRj aEV2ZW50KFBhc3NSZWZQCiAgICAgICAgIGVjID0gRXZlbnRFeGNlcHRpb246OlVOU1BFQ0lGSUVE X0VWRU5UX1RZUEVfRVJSOwogICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICB9CisKKyAgICBlYyA9 IDA7CiAgICAgCiAgICAgTGlzdGVuZXJWZWN0b3IgbGlzdGVuZXJzQ29weSA9IG1fZXZlbnRMaXN0 ZW5lcnMuZ2V0KGV2ZW50LT50eXBlKCkuaW1wbCgpKTsKICAgICBmb3IgKExpc3RlbmVyVmVjdG9y Ojpjb25zdF9pdGVyYXRvciBsaXN0ZW5lckl0ZXIgPSBsaXN0ZW5lcnNDb3B5LmJlZ2luKCk7IGxp c3RlbmVySXRlciAhPSBsaXN0ZW5lcnNDb3B5LmVuZCgpOyArK2xpc3RlbmVySXRlcikgewpJbmRl eDogV2ViQ29yZS9wYWdlL0RPTVdpbmRvdy5jcHANCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBXZWJDb3JlL3Bh Z2UvRE9NV2luZG93LmNwcAkocmV2aXNpb24gMzc5NDgpCisrKyBXZWJDb3JlL3BhZ2UvRE9NV2lu ZG93LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzU2LDYgKzM1Niw4IEBAIHZvaWQgRE9NV2luZG93 Ojpwb3N0TWVzc2FnZShjb25zdCBTdHJpbmcKICAgICBpZiAoIW1fZnJhbWUpCiAgICAgICAgIHJl dHVybjsKIAorICAgIGVjID0gMDsKKwogICAgIC8vIENvbXB1dGUgdGhlIHRhcmdldCBvcmlnaW4u ICBXZSBuZWVkIHRvIGRvIHRoaXMgc3luY2hyb25vdXNseSBpbiBvcmRlcgogICAgIC8vIHRvIGdl bmVyYXRlIHRoZSBTWU5UQVhfRVJSIGV4Y2VwdGlvbiBjb3JyZWN0bHkuCiAgICAgUmVmUHRyPFNl Y3VyaXR5T3JpZ2luPiB0YXJnZXQ7Cg== 24737 2008-10-28 16:50:06 -0700 2008-10-29 08:52:25 -0700 Updated patch. MessagePort.ecinit.patch text/plain 2042 mbelshe SW5kZXg6IENoYW5nZUxvZw0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQ0KLS0tIENoYW5nZUxvZwkocmV2aXNpb24gMzc5 NDkpCisrKyBDaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwyMyBAQAorMjAwOC0x MC0yOCAgbWlrZUBiZWxzaGUuY29tCisgICAgICAgIEZpeCB1bmluaXRpYWxpemVkIEV4Y2VwdGlv bkNvZGVzIGluIE1lc3NhZ2VQb3J0LgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgICogQ2hhbmdlTG9nCis6CisgICAgICAgICogV2ViQ29yZVxkb21cTWVz c2FnZVBvcnQuY3BwCis6CisgICAgICAgICogV2ViQ29yZVxwYWdlXERPTVdpbmRvdy5jcHAKKzoK IDIwMDgtMTAtMjggIE1pa2UgSG9tbWV5ICA8Z2xhbmRpdW1AZGViaWFuLm9yZz4KIAogICAgICAg ICBSZXZpZXdlZCBieSBBbHAgVG9rZXIuCkluZGV4OiBXZWJDb3JlL2RvbS9NZXNzYWdlUG9ydC5j cHANCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0NCi0tLSBXZWJDb3JlL2RvbS9NZXNzYWdlUG9ydC5jcHAJKHJldmlzaW9u IDM3OTQ4KQorKysgV2ViQ29yZS9kb20vTWVzc2FnZVBvcnQuY3BwCSh3b3JraW5nIGNvcHkpCkBA IC05NSw2ICs5NSw4IEBAIFBhc3NSZWZQdHI8TWVzc2FnZVBvcnQ+IE1lc3NhZ2VQb3J0OjpjbG8K ICAgICAgICAgcmV0dXJuIDA7CiAgICAgfQogCisgICAgZWMgPSAwOworCiAgICAgUmVmUHRyPE1l c3NhZ2VQb3J0PiByZW1vdGVQb3J0ID0gbV9lbnRhbmdsZWRQb3J0OwogICAgIFJlZlB0cjxNZXNz YWdlUG9ydD4gbmV3UG9ydCA9IE1lc3NhZ2VQb3J0OjpjcmVhdGUobmV3T3duZXIpOwogCkBAIC0x MTUsNiArMTE3LDggQEAgdm9pZCBNZXNzYWdlUG9ydDo6cG9zdE1lc3NhZ2UoY29uc3QgU3RyaQog CiB2b2lkIE1lc3NhZ2VQb3J0Ojpwb3N0TWVzc2FnZShjb25zdCBTdHJpbmcmIG1lc3NhZ2UsIE1l c3NhZ2VQb3J0KiBkYXRhUG9ydCwgRXhjZXB0aW9uQ29kZSYgZWMpCiB7CisgICAgZWMgPSAwOwor CiAgICAgaWYgKCFtX2VudGFuZ2xlZFBvcnQgfHwgIW1fZG9jdW1lbnQgfHwgIW1fZW50YW5nbGVk UG9ydC0+bV9kb2N1bWVudCkKICAgICAgICAgcmV0dXJuOwogCkBAIC0yOTMsNiArMjk3LDggQEAg Ym9vbCBNZXNzYWdlUG9ydDo6ZGlzcGF0Y2hFdmVudChQYXNzUmVmUAogICAgICAgICBlYyA9IEV2 ZW50RXhjZXB0aW9uOjpVTlNQRUNJRklFRF9FVkVOVF9UWVBFX0VSUjsKICAgICAgICAgcmV0dXJu IHRydWU7CiAgICAgfQorCisgICAgZWMgPSAwOwogICAgIAogICAgIExpc3RlbmVyVmVjdG9yIGxp c3RlbmVyc0NvcHkgPSBtX2V2ZW50TGlzdGVuZXJzLmdldChldmVudC0+dHlwZSgpLmltcGwoKSk7 CiAgICAgZm9yIChMaXN0ZW5lclZlY3Rvcjo6Y29uc3RfaXRlcmF0b3IgbGlzdGVuZXJJdGVyID0g bGlzdGVuZXJzQ29weS5iZWdpbigpOyBsaXN0ZW5lckl0ZXIgIT0gbGlzdGVuZXJzQ29weS5lbmQo KTsgKytsaXN0ZW5lckl0ZXIpIHsKSW5kZXg6IFdlYkNvcmUvcGFnZS9ET01XaW5kb3cuY3BwDQo9 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09DQotLS0gV2ViQ29yZS9wYWdlL0RPTVdpbmRvdy5jcHAJKHJldmlzaW9uIDM3OTQ4 KQorKysgV2ViQ29yZS9wYWdlL0RPTVdpbmRvdy5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTM1Niw2 ICszNTYsOCBAQCB2b2lkIERPTVdpbmRvdzo6cG9zdE1lc3NhZ2UoY29uc3QgU3RyaW5nCiAgICAg aWYgKCFtX2ZyYW1lKQogICAgICAgICByZXR1cm47CiAKKyAgICBlYyA9IDA7CisKICAgICAvLyBD b21wdXRlIHRoZSB0YXJnZXQgb3JpZ2luLiAgV2UgbmVlZCB0byBkbyB0aGlzIHN5bmNocm9ub3Vz bHkgaW4gb3JkZXIKICAgICAvLyB0byBnZW5lcmF0ZSB0aGUgU1lOVEFYX0VSUiBleGNlcHRpb24g Y29ycmVjdGx5LgogICAgIFJlZlB0cjxTZWN1cml0eU9yaWdpbj4gdGFyZ2V0Owo= 28229 2009-03-03 10:48:20 -0800 2009-03-03 22:46:46 -0800 revised patch bug21939.patch text/plain 874 mbelshe SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0MTM5NCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTAgQEAKKzIwMDktMDMtMDMgIE1pa2UgQmVsc2hlICA8bWlrZUBiZWxzaGUuY29t PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogZG9t L01lc3NhZ2VQb3J0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1lc3NhZ2VQb3J0Ojpwb3N0TWVz c2FnZSk6CisKIDIwMDktMDMtMDMgIERpbWl0cmkgR2xhemtvdiAgPGRnbGF6a292QGNocm9taXVt Lm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBEYXJpbiBGaXNoZXIuCkluZGV4OiBXZWJDb3Jl L2RvbS9NZXNzYWdlUG9ydC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9kb20vTWVzc2FnZVBv cnQuY3BwCShyZXZpc2lvbiA0MTM4NCkKKysrIFdlYkNvcmUvZG9tL01lc3NhZ2VQb3J0LmNwcAko d29ya2luZyBjb3B5KQpAQCAtMTM3LDYgKzEzNyw3IEBAIHZvaWQgTWVzc2FnZVBvcnQ6OnBvc3RN ZXNzYWdlKGNvbnN0IFN0cmkKICAgICAgICAgICAgIGVjID0gSU5WQUxJRF9BQ0NFU1NfRVJSOwog ICAgICAgICAgICAgcmV0dXJuOwogICAgICAgICB9CisgICAgICAgIGVjID0gMDsKICAgICAgICAg bmV3TWVzc2FnZVBvcnQgPSBkYXRhUG9ydC0+Y2xvbmUoZWMpOwogICAgICAgICBpZiAoZWMpCiAg ICAgICAgICAgICByZXR1cm47Cg==