417075 - Update postMessage and MessageEvent to reflect domain/uri being replaced by origin, optional origin argument

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Jeff Walden [:Waldo]]

origin/uri properties were removed from MessageEvent because uri communicates username/password and domain doesn't include a port, which is insufficient for shared-host situations where different ports are controlled by different entities.  This change is fairly simple; I almost have a working patch, and most of the changes are to tests.

Comment 1

[Adam Barth]

The postMessage API now also takes an optional "origin" parameter to protect the confidentiality of messages:

http://html5.org/tools/web-apps-tracker?from=1216&to=1217

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch

Apologies about the patch size; this basically required me to make changes to every test using postMessage.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 302961 [details] [diff] [review]
Patch

Blah, there are intricate steps in the spec now, and I haven't checked whether we exactly follow them or not yet.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch, follows spec directions, more tests, tests, tests!

So step 2 can basically be summarized as: if a non-null origin is given, throw a syntax error if it's not a valid URI, and otherwise if that URI's scheme, host, and port don't match those of the target document return silently.  (There's some differences with respect to data: URLs and how they have a globally unique identifier as an origin, but I can't quite see us changing that at this stage in the release cycle.)

The todos in that test are basically either NS_NewURI accepting non-URI input or our nsStandardURL-based IDN spoofing solution affecting the computed value of an origin with respect to punycoding (bug 414090).  If there's a stricter URI-creation function than NS_NewURI, I'd love to know about it; see the list of tests in test_postMessage_origin.xhtml whose {...} object literals contain an empty line.  The current set of non-IDN failure messages is:

todo - should throw on test #4   -  "http: //"
todo - should throw on test #6   -  " http://localhost:8888"
todo - should throw on test #9   -  "http: //localhost:8888"
todo - should throw on test #11  -  "http:// localhost:8888"
todo - should throw on test #12  -  "http://\nlocalhost:8888"
todo - should throw on test #13  -  "http://localhost:8888\0"
todo - should throw on test #14  -  "http://localhost:8888\n"

I'm not 100% sure that all of these are invalid URIs, so a quick double-check would be nice.

This patch relies on the patch in bug 417218 for the tests with the "standardURLBug" property to work correctly.  Without that patch, I could add:

  else if (test.standardURLBug)
    todo(called, "should have been called for test #" + i);

and I believe they'd show up as todos.  (I had this briefly in my tree until I created the patch, at which point I removed it to be sure it worked correctly, and I didn't copy the exact code I added.)

Comment 5

[Robert Sayre]

Either this blocks, or we cut postMessage. Given the test backup, we should take it, imho.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 303214 [details] [diff] [review]
Patch, follows spec directions, more tests, tests, tests!

>Index: dom/src/base/nsGlobalWindow.cpp

>+    if (NS_FAILED(targetOrigin->Clone(getter_AddRefs(targetOrigin))) ||

This isn't safe on all compilers. The evaluation order between |targetOrigin->| and |getter_AddRefs(targetOrigin)| is undefined. If the latter executes before there will be badness since getter_AddRefs nulls out its argument.

Same thing for callerOrigin.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 303214 [details] [diff] [review]
Patch, follows spec directions, more tests, tests, tests!

I've changed the two to *URI and then cloned to *Origin locally but don't have the time right now to rebuild and rerun Mochitests; will do so before checkin, but want to get the approval request filed as early as possible.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 303214 [details] [diff] [review]
Patch, follows spec directions, more tests, tests, tests!

a=beltzner for 1.9

Comment 9

[Jeff Walden [:Waldo]]

Fixed.

Comment 10

[Jeff Walden [:Waldo]]

I updated the postMessage article to describe the new syntax.