Bug 21939 - Uninitialized ExceptionCode in DOMWindow::postMessage
Summary: Uninitialized ExceptionCode in DOMWindow::postMessage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mike Belshe
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-28 16:44 PDT by Mike Belshe
Modified: 2009-03-03 23:20 PST (History)
2 users (show)

See Also:


Attachments
Initialize the exceptioncodes. (1.52 KB, patch)
2008-10-28 16:45 PDT, Mike Belshe
no flags Details | Formatted Diff | Diff
Updated patch. (1.99 KB, patch)
2008-10-28 16:50 PDT, Mike Belshe
no flags Details | Formatted Diff | Diff
revised patch (874 bytes, patch)
2009-03-03 10:48 PST, Mike Belshe
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Belshe 2008-10-28 16:44:29 PDT
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.
Comment 1 Mike Belshe 2008-10-28 16:45:58 PDT
Created attachment 24736 [details]
Initialize the exceptioncodes.
Comment 2 Mike Belshe 2008-10-28 16:50:06 PDT
Created attachment 24737 [details]
Updated patch.

Same patch as before, but this one contains a changelog.
Comment 3 Darin Adler 2008-10-28 16:54:27 PDT
Comment on attachment 24737 [details]
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.
Comment 4 Alexey Proskuryakov 2008-10-29 03:50:45 PDT
And the caller (JSDOMWindow::postMessage()) does initialize ec - maybe the issue you are seeing is V8-specific?
Comment 5 Mike Belshe 2008-10-29 08:45:18 PDT
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.
Comment 6 Mike Belshe 2008-10-29 08:45:55 PDT
Comment on attachment 24737 [details]
Updated patch.

See my comments about the changes to DOMWindow.cpp, which I think is still a valid bug.
Comment 7 Mike Belshe 2008-10-29 08:52:54 PDT
OK - nevermind.  Alexey - I see what you are saying.  

We want the declarer to initialize.
Comment 8 Mike Belshe 2008-10-29 09:03:54 PDT
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?
Comment 9 Darin Adler 2008-10-29 09:20:23 PDT
(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.
Comment 10 Alexey Proskuryakov 2008-10-29 11:05:00 PDT
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?
Comment 11 Alexey Proskuryakov 2008-10-29 11:24:02 PDT
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.
Comment 12 Darin Adler 2008-10-29 13:06:03 PDT
(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().
Comment 13 Darin Adler 2008-10-29 13:07:27 PDT
(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.
Comment 14 Alexey Proskuryakov 2008-10-29 14:01:51 PDT
I see, thank you for the clarification!
Comment 15 Mike Belshe 2009-03-03 10:48:20 PST
Created attachment 28229 [details]
revised patch

Finally cycled back to this bug; revised the patch just to contain the one ec initialization which we agreed was important.
Comment 16 Alexey Proskuryakov 2009-03-03 22:46:46 PST
Comment on attachment 28229 [details]
revised patch

r=me (I hope Darin doesn't mind!)

Please be sure to add bug URL and title to ChangeLog when landing.
Comment 17 Darin Fisher (:fishd, Google) 2009-03-03 23:20:28 PST
Landed as http://trac.webkit.org/changeset/41418