sendBacon would be even more delicious! :-)

I hope the spec changes a bunch, but this is my wip on it so far.

Not sure, but shouldn't this call nsIContentPolicy with TYPE_PING or a new TYPE_BEACON?

@devd YES! good suggestion.

for those following along, https://github.com/dougt/gecko-dev/tree/bug_936340_navigator_sendBeacon

i would like feedback on the patch so far.

Comment on attachment 8351139 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.-r.patch Sicking, there are some "TODO"'s in the code. Most are probably spec issues. Can you provide feedback?

Comment on attachment 8351139 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.-r.patch Review of attachment 8351139 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsMixedContentBlocker.cpp @@ +241,4 @@ > // and other advanced CSS features can possibly be exploited to cause > // spoofing attacks (e.g. make a "grant permission" button look like a > // "refuse permission" button). > + // Whitespace at EOL ::: dom/base/Navigator.cpp @@ +989,5 @@ > + > + NS_DECL_ISUPPORTS > + NS_DECL_NSISTREAMLISTENER > + NS_DECL_NSIREQUESTOBSERVER > + Whitespace at EOL. @@ +1023,5 @@ > + nsISupports *aContext) > +{ > + printf("BeaconStreamListener::OnStartRequest\n"); > + > + HeaderVisitor *visitor = new HeaderVisitor(); Needs to be a strong reference. @@ +1047,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + printf("BeaconStreamListener::OnStopRequest (status: %d)\n", httpStatus); > + return NS_OK; > +} Everything above here looks like debugging code. Remove it? @@ +1059,5 @@ > +{ > + printf("BeaconStreamListener::ODA\n"); > + uint32_t totalRead; > + return inStr->ReadSegments(NS_DiscardSegment, nullptr, count, &totalRead); > +} Do we have to explicitly discard the response data? Can we pass null for the stream listener? Per spec UAs are allowed to close the connection once they start to receive a response ... @@ +1063,5 @@ > +} > + > +void > +Navigator::SendBeacon(const nsAString& aUrl, > + const Nullable<ArrayBufferViewOrBlobOrStringOrFormData >& aData, Whitespace before > again. @@ +1070,5 @@ > + if (aData.IsNull()) { > + //TODO: I tend to think you should be able to send null data. > + aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); > + return; > + } null should either be allowed here or disallowed in the WebIDL. This is a spec bug. @@ +1102,5 @@ > + aRv.Throw(NS_ERROR_DOM_URL_MISMATCH_ERR); > + return; > + } > + > + //TODO: why would we ever want this to work over http. This should be handled by content policy, IMO. @@ +1122,5 @@ > + return; > + } > + } > + > + //TODO do we want to worry about beaconing from https to http? This should be handled by mixed content. @@ +1175,5 @@ > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("accept-language"), > + EmptyCString(), false); > + > + httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("accept-encoding"), > + EmptyCString(), false); Why are we setting all these? @@ +1180,5 @@ > + > + nsCString mimeType; > + nsCOMPtr<nsIInputStream> in; > + > + if (aData.Value().IsString()) { We should factor out the relevant parts of nsXMLHttpRequest::GetRequestBody and use it here. @@ +1224,5 @@ > + charset); > + } else { > + aRv.Throw(NS_ERROR_FAILURE); > + return; > + } It should be impossible to get to the else branch, no? @@ +1227,5 @@ > + return; > + } > + > + nsCOMPtr<nsIUploadChannel> uploadChannel = do_QueryInterface(channel); > + uploadChannel->SetUploadStream(in, mimeType, -1); You should use nsIUploadChannel2. ::: dom/base/Navigator.h @@ +7,5 @@ > #ifndef mozilla_dom_Navigator_h > #define mozilla_dom_Navigator_h > > +#include "mozilla/dom/Nullable.h" > +#include "mozilla/dom/UnionTypes.h" The forward decl should be enough. You should not need the UnionTypes.h include. @@ +226,5 @@ > system::AudioChannelManager* GetMozAudioChannelManager(ErrorResult& aRv); > #endif // MOZ_AUDIO_CHANNEL_MANAGER > + > + void SendBeacon(const nsAString& aUrl, > + const Nullable<ArrayBufferViewOrBlobOrStringOrFormData >& aData, No whitespace before > ::: dom/bindings/BindingDeclarations.h @@ +20,4 @@ > #include "nsCOMPtr.h" > #include "nsTArray.h" > #include "nsAutoPtr.h" // for nsRefPtr member variables > +#include "nsIDOMFile.h" Forward declare if you need this. ::: dom/bindings/Bindings.conf @@ +147,5 @@ > > +'Beacon': { > + 'nativeType': 'nsDOMBeacon', > + 'headerFile': 'Beacon.h', > +}, Name your files/types correctly so that this is not necessary. ::: dom/webidl/Beacon.webidl @@ +12,5 @@ > + > +partial interface Navigator { > + [Throws, Pref="beacon.enabled"] > + > + void sendBeacon(DOMString url, No newline above this. @@ +13,5 @@ > +partial interface Navigator { > + [Throws, Pref="beacon.enabled"] > + > + void sendBeacon(DOMString url, > + (ArrayBufferView or Blob or DOMString or FormData)? data); This method is supposed to return a boolean per spec. @@ +14,5 @@ > + [Throws, Pref="beacon.enabled"] > + > + void sendBeacon(DOMString url, > + (ArrayBufferView or Blob or DOMString or FormData)? data); > +}; This should all go in Navigator.webidl. We create new WebIDL files for new interfaces, not for partial additions to existing interfaces. ::: test.sh @@ +1,1 @@ > +MOZCONFIG=mozconfig.ff DISPLAY=:1 ./mach mochitest-plain dom/tests/mochitest/beacon/ Uh, no :)

Comment on attachment 8351139 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.-r.patch Review of attachment 8351139 [details] [diff] [review]: ----------------------------------------------------------------- These comments are in addition to khuey's. ::: content/base/public/nsIContentPolicy.idl @@ +147,5 @@ > > + /** > + * Indicates a beacon post. > + */ > + const nsContentPolicyType TYPE_BEACON = 19; IIRC you need to rev the uuid of this interface to make sure that the new constant will show up in the xpt for incremental builds. ::: dom/base/Navigator.cpp @@ +998,5 @@ > + NS_DECL_ISUPPORTS > + NS_DECL_NSIHTTPHEADERVISITOR > + > + HeaderVisitor() { } > + virtual ~HeaderVisitor() {} The virtual dtor is not needed. @@ +1013,5 @@ > +NS_IMETHODIMP > +BeaconStreamListener::HeaderVisitor::VisitHeader(const nsACString &header, const nsACString &value) > +{ > + printf("BeaconStreamListener::HeaderVisitor %s: %s\n", > + PromiseFlatCString(header).get(), PromiseFlatCString(value).get()); If this class is only used for debugging purposes, please remove it. @@ +1027,5 @@ > + HeaderVisitor *visitor = new HeaderVisitor(); > + > + nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aRequest)); > + NS_ENSURE_TRUE(httpChannel, NS_ERROR_FAILURE); > + httpChannel->VisitRequestHeaders(visitor); Again, all of this code can go away if HeaderVisitor is debug-only. @@ +1059,5 @@ > +{ > + printf("BeaconStreamListener::ODA\n"); > + uint32_t totalRead; > + return inStr->ReadSegments(NS_DiscardSegment, nullptr, count, &totalRead); > +} Yeah, I also think we should close the connection here instead. @@ +1086,5 @@ > + > + nsIURI* documentURI = doc->GetDocumentURI(); > + nsIURI* baseURI = doc->GetDocBaseURI(); > + nsCString charset = doc->GetDocumentCharacterSet(); > + charset = doc->GetDocumentCharacterSet(); Please remove this line. @@ +1133,5 @@ > + } > + > + rv = nsContentUtils::GetSecurityManager()->CheckSameOrigin(cx, uri); > + if (NS_FAILED(rv)) { > + aRv.Throw(NS_ERROR_DOM_SECURITY_ERR); Hmm, the spec says that we should make a CORS request here. Am I missing something? @@ +1220,5 @@ > + nsAutoCString charset; > + form.GetSendInfo(getter_AddRefs(in), > + &len, > + mimeType, > + charset); Shouldn't we convert the form data to UTF-8 here? ::: dom/bindings/Bindings.conf @@ +147,5 @@ > > +'Beacon': { > + 'nativeType': 'nsDOMBeacon', > + 'headerFile': 'Beacon.h', > +}, Hmm, neither of these files/classes appear in the patch. And there is no Beacon WebIDL interface either. Please remove this. ::: dom/tests/mochitest/beacon/beacon-frame.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Frame for watchPosition </title> sendBeacon @@ +11,5 @@ > +{ > + var frame = window.parent.document.getElementById("frame"); > + var data = window.parent.beaconConvert(frame.getAttribute("data")); > + > + navigator.sendBeacon("http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-handler.sjs", data); Please also test calling sendBeacon with a relative URI. ::: dom/tests/mochitest/beacon/test_beacon.html @@ +18,5 @@ > +<script class="testbody" type="text/javascript"> > + > +// not enabled by default yet. > +var prefs = SpecialPowers.Cc["@mozilla.org/preferences-service;1"].getService(SpecialPowers.Ci.nsIPrefBranch); > +prefs.setBoolPref("beacon.enabled", true); Use SpecialPowers.pushPrefEnv instead. @@ +20,5 @@ > +// not enabled by default yet. > +var prefs = SpecialPowers.Cc["@mozilla.org/preferences-service;1"].getService(SpecialPowers.Ci.nsIPrefBranch); > +prefs.setBoolPref("beacon.enabled", true); > + > +SimpleTest.waitForExplicitFinish(); This test is synchronous so you can remove this and the finish call below. ::: dom/tests/mochitest/beacon/test_beaconContentPolicy.html @@ +21,5 @@ > +const Ci = SpecialPowers.Ci; > + > +// not enabled by default yet. > +var prefs = SpecialPowers.Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch); > +prefs.setBoolPref("beacon.enabled", true); Ditto. @@ +73,5 @@ > +} > + > +function teardownPolicy() { > + setTimeout(function() { > + // policy will not be removed from the category correctly ?? ::: dom/tests/mochitest/beacon/test_beaconFrame.html @@ +19,5 @@ > +<script class="testbody" type="text/javascript"> > + > +// not enabled by default yet. > +var prefs = SpecialPowers.Cc["@mozilla.org/preferences-service;1"].getService(SpecialPowers.Ci.nsIPrefBranch); > +prefs.setBoolPref("beacon.enabled", true); Same here.

Thanks for the feedback all. I'll remove the testing code, and simplify the stream listener including cancelling the request from ODA or OnStart. > // policy will not be removed from the category correctly Yeah - xpcom fail. :( > Hmm, the spec says that we should make a CORS request here. Am I missing something? Yes: """ Otherwise, make a cross-origin request to URL, using the POST HTTP method with transmittedData, encoding, and mime type. """

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10) > IIRC you need to rev the uuid of this interface to make sure that the new > constant will show up in the xpt for incremental builds. What?! That's crazy. Is there a bug on file?

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #12) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #10) > > IIRC you need to rev the uuid of this interface to make sure that the new > > constant will show up in the xpt for incremental builds. > > What?! That's crazy. Is there a bug on file? I can't find the bug right now, Kyle may remember what I'm talking about. I think it had something to do with const interface values to not be picked up in the xpt if there was no uuid change on the interface...

jduell, looking for feedback on the networking part of this.

Comment on attachment 8356969 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.-r.patch ehsan, looking for feedback on the CORS stuff. This is new to me. I need to write a test for the CORS stuff (that is why this is a feedback, not a review request).

Comment on attachment 8356969 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.-r.patch Review of attachment 8356969 [details] [diff] [review]: ----------------------------------------------------------------- CSP changes look appropriate. Glad you saw the comment in nsIContentPolicy :). CSP will not block TYPE_BEACON by default (only possibly on websites that use a CSP). ::: content/base/src/contentSecurityPolicy.js @@ +106,4 @@ > csp._MAPPINGS[cp.TYPE_XBL] = cspr_sd_new.DEFAULT_SRC; > csp._MAPPINGS[cp.TYPE_PING] = cspr_sd_new.DEFAULT_SRC; > csp._MAPPINGS[cp.TYPE_DTD] = cspr_sd_new.DEFAULT_SRC; > + csp._MAPPINGS[cp.TYPE_BEACON] = cspr_sd_new.DEFAULT_SRC; Adding this to the DEFAULT_SRC directive means that if a document has a CSP and the policy does not specify default-src, no beacons will be blocked. I think this is fine, but just a FYI. ::: content/base/src/nsMixedContentBlocker.cpp @@ +287,4 @@ > case TYPE_MEDIA: > case TYPE_OBJECT_SUBREQUEST: > case TYPE_PING: > + case TYPE_BEACON: If you want TYPE_BEACON to be blocked by mixed content blocker by default (https page, http beacon, default Firefox settings) then you need TYPE_BEACON to be eMixedScript. Also see nsMixedContentBlocker.cpp:232 for notes on why PING is mixed display (not mixed script).

I disagree with including ping and sendBeacon into CSP default-src. CSP is about a policy on what is loaded onto the page and not about controlling all data exfiltration channels. dveditz puts it as: "[sendBeacon/]ping is explicitly a replacement for a navigation redirect chain, and CSP doesn't govern navigation" I fear that requiring sendBeacon targets be whitelisted with default-src could cause sites to adopt a weaker default-src setting for their page or negatively impact adoption of sendBeacon & CSP. Similarly, I think mixedContentBlocker should not do anything about sendBeacon; the response is never loaded onto the page. Again, the comparison to navigations is apt. One thing to check would be making sure that a HTTP request for sendBeacon doesn't include the HTTPS referer (the current patch should already have this behavior I believe)

Comment on attachment 8356969 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.-r.patch Review of attachment 8356969 [details] [diff] [review]: ----------------------------------------------------------------- The CORS stuff looks good, didn't look closely at the rest of the patch (let me know if you want me to!) ::: dom/base/Navigator.cpp @@ +1167,5 @@ > + > + nsRefPtr<nsIStreamListener> listener = new BeaconStreamListener(); > + nsRefPtr<nsCORSListenerProxy> cors = new nsCORSListenerProxy(listener, > + doc->NodePrincipal(), > + false); I assume that withCredentials=false is fine spec-wise.

(In reply to Devdatta Akhawe [:devd] from comment #17) You raise some good points ! > CSP is about a policy on what is loaded onto the page and not about controlling all > data exfiltration channels. this isn't entirely true in the CSP 1.1 era, imo - form-action in CSP 1.1 for example has nothing to do with what's loaded on the page. Neither does connect-src (formerly xhr-src) or frame-ancestors. popup-src is another directive that restricts navigation in auxiliary browsing contexts. CSP has gone beyond merely where a page is allowed to load data from. > dveditz puts it as: > > "[sendBeacon/]ping is explicitly a replacement for a navigation redirect > chain, and CSP doesn't govern navigation" I don't agree about sendBeacon being about navigation redirect necessarily - one major use case is to avoid a sync XHR on page unload as stated in the spec. > I fear that requiring sendBeacon targets be whitelisted with default-src > could cause sites to adopt a weaker default-src setting for their page or > negatively impact adoption of sendBeacon & CSP. I think this is definitely a valid concern. It would make more sense to me to have it be restricted by connect-src or something along those lines perhaps - This seems like a discussion for the w3c list IMO. > Similarly, I think mixedContentBlocker should not do anything about > sendBeacon; the response is never loaded onto the page. Again, the > comparison to navigations is apt. One thing to check would be making sure > that a HTTP request for sendBeacon doesn't include the HTTPS referer (the > current patch should already have this behavior I believe) Mixed content blocker similarly restricts XHR as active content, I'm not sure that's appropriate here but my point is that mixed content blocker already restricts similar things that don't necessarily load content on to the page (imagine something sensitive is being sent via sendBeacon, for a example, from an HTTPS page to an HTTP URL).

Comment on attachment 8356969 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.-r.patch Review of attachment 8356969 [details] [diff] [review]: ----------------------------------------------------------------- Looks good mostly, but I'll be curious to see whether this works on e10s--in order to do a channel on e10s you've got to have a TabChild (docShell) available to get security info, and sendBeacon might be operating in that weird time interval when we don't seem to always have one. See for example, bug 833935, where bz/jdm seemed to conclude it was easier to work around this issue by simply failing AsyncOpen. At least you're throwing if AsyncOpen fails, so presumably your test will detect if this winds up being an issue. I actually think you should do a s/Beacon/Bacon/g on this patch. The tree has been seriously lacking in whimsy recently. :) ::: dom/base/Navigator.cpp @@ +1001,5 @@ > +NS_IMETHODIMP > +BeaconStreamListener::OnStartRequest(nsIRequest *aRequest, > + nsISupports *aContext) > +{ > + return NS_OK; move Cancel here (see comment below). @@ +1024,5 @@ > + // terminate when the data has been sent from the UA to the server. That is, > + // we do not have to completely read the result of the POST. Is there a > + // better error code to Cancel() for this? > + aRequest->Cancel(NS_ERROR_NET_INTERRUPT); > + return NS_OK; The XHR code uses Cancel(NS_OK) to indicate that it doesn't want to keep reading from the channel but there's no error (so OnStopRequest is passed NS_OK for status): http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1829 That said, I'm not sure that idiom will work with all channel types--it requires the channel to check both mCanceled and mStatus (and there may be places that just check NS_FAILED(mStatus). Ah, right, the IDL says to avoid passing NS_OK: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIRequest.idl#63 So pass a failure code. I'd prefer NS_BINDING_ABORTED (NET_INTERRUPT is generally set by the network layer, ABORT is for clients). Also, note that you're not reading the bytes here, and you'd need to do that (the XHR code above reads the stream before it cancel()'s), else you'll print error msgs to the web console (bug 266532). Just do the Cancel() in OnStartRequest, then ODA will never get called (put a MOZ_ASSERT(false) for the body here). ::: dom/tests/mochitest/beacon/test_beaconContentPolicy.html @@ +87,5 @@ > + }, 0); > +} > + > +function beginTest() { > + navigator.sendBeacon(beaconUrl, "bob moss is my boss"); Warning--this line could eventually suffer from org-rot.

I brought up Beacon and CSP on the W3C webappsec list - the thread is at http://lists.w3.org/Archives/Public/public-webappsec/2014Jan/0104.html TL;DR either form-action or connect-src seem like they would be appropriate. Note that form-action is in CSP 1.1 and I don't believe that it is implemented for CSP in Gecko yet.

FWIW, Anne has now convinced me that sendBeacon should be restricted by the CSP connect-src directive (not form-action). See http://lists.w3.org/Archives/Public/public-webappsec/2014Jan/0135.html

Diffs from Turner's patch: * Change CSP directive to connect-src * Move Cancel call to OnStartRequest * Add NS_CheckContentLoadPolicy to make CSP actually work * Add third party check so that cookie policies work without needing to have the window around * Change withCredentials to true so that cookies get sent * Add a test for CORS preflight with a weird content type * Add a test for cookie handling on sendBeacon requests

Comment on attachment 8368222 [details] [diff] [review] 0001-Bug-936340-Implement-navigator.sendBeacon.patch Review of attachment 8368222 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good. Just some minor fixes. ::: content/base/public/nsIContentPolicy.idl @@ -24,4 @@ > * by launching a dialog to prompt the user for something). > */ > > -[scriptable,uuid(e48e3024-f302-4a16-b8b6-2034d3a4b279)] I don't think you need to change the iid here since the interface is still binary compatible with the old one. ::: dom/base/Navigator.cpp @@ +1004,5 @@ > +NS_IMETHODIMP > +BeaconStreamListener::OnStartRequest(nsIRequest *aRequest, > + nsISupports *aContext) > +{ > + aRequest->Cancel(NS_ERROR_NET_INTERRUPT); Potentially it's better to let the request keep going so that it can be reused. Ask the Necko team what the optimal solution is here. @@ +1023,5 @@ > + nsIInputStream *inStr, > + uint64_t sourceOffset, > + uint32_t count) > +{ > + // TODO - ask necko peers about this. The spec allows us to immediately Seems like you decided to cancel in OnStartRequest? Either way this comment should be reviewed by the necko team. @@ +1037,5 @@ > + const Nullable<ArrayBufferViewOrBlobOrStringOrFormData>& aData, > + ErrorResult& aRv) > +{ > + if (aData.IsNull()) { > + aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); Why do we not allow submitting null data? That seems useful. Is the idea to require an empty string? @@ +1064,5 @@ > + nsCOMPtr<nsIURI> uri; > + nsresult rv = NS_NewURI(getter_AddRefs(uri), > + aUrl, > + charset.get(), > + baseURI); Use nsContentUtils::NewURIWithDocumentCharset It would be super awesome if you could add a function to nsContentUtils which takes a relative url in the form of a string, and a document and then grabs both baseuri and charset from the document and returns a new URI. I.e. something like nsContentUtils::NewURIWithDocumentCharset but which doesn't need a separate baseURI. This is a really common pattern so it's silly that we keep repeating it. @@ +1073,5 @@ > + > + // Check whether the CSP allows us to load > + int16_t shouldLoad = nsIContentPolicy::ACCEPT; > + nsCOMPtr<nsIPrincipal> principal = doc->NodePrincipal(); > + rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_BEACON, You need to add a nsIScriptSecurityManager::CheckLoadURI check before this call. To prevent sending a beacon to things like chrome:// and file:// @@ +1135,5 @@ > + return false; > + } > + bool isForeign = true; > + thirdPartyUtil->IsThirdPartyWindow(mWindow, uri, &isForeign); > + httpChannelInternal->SetForceAllowThirdPartyCookie(!isForeign); Please get someone else to review this part. In particular, will this still do the right thing even if the load is redirected? @@ +1205,5 @@ > + nsRefPtr<nsCORSListenerProxy> cors = new nsCORSListenerProxy(listener, > + principal, > + true); > + > + // Start a preflight if cross-origin and content type is not whitelisted Nit: End-of-line whitespace @@ +1208,5 @@ > + > + // Start a preflight if cross-origin and content type is not whitelisted > + nsAutoCString documentOrigin, uriOrigin; > + rv = documentURI->GetPrePath(documentOrigin); > + uri->GetPrePath(uriOrigin); Use nsIScriptSecurityManager::CheckSameOriginURI here. In general you never want to create an origin yourself. Use nsContentUtils::GetASCIIOrigin or nsContentUtils::GetUTFOrigin if you really need the origin as a string. But be mindfull that they might return "null" as string. Which is why using nsIScriptSecurityManager::CheckSameOriginURI is always better. @@ +1218,5 @@ > + !contentType.Equals(TEXT_PLAIN)) { > + nsCOMPtr<nsIChannel> preflightChannel; > + nsTArray<nsCString> unsafeHeaders; > + unsafeHeaders.AppendElement(NS_LITERAL_CSTRING("Content-Type")); > + rv = NS_StartCORSPreflight(channel, listener, I think you want to pass 'cors' here as the second argument. In general, it's probably better that you don't keep a reference to the beacon listener directly, but only keep a reference to the cors listener. I.e. do nsCOMPtr<nsIStreamListener> listener = new nsCORSListenerProxy(new BeaconStreamListener(), principal, true);

Latest patch has responses to Sicking review. Handled: * Reverted IID on nsIContentPolicy * Removed comment about placement of Cancel (already reviewed by jduell) * Now use nsContentUtils::NewURIWithDocumentCharset * Perform check with nsIScriptSecurityManager::CheckLoadURIWithPrincipal * Requesting mmc review of cookie foreignness handling * Removed EOL whitespace * Use nsIScriptSecurityManager::CheckSameOriginURI for cross-origin check * Allow input of null data * NOTE: This causes WebIDL to diverge from spec; need to file spec bug Deferred: * Implementing new method nsContentUtils::NewURIWithDocument

Comment on attachment 8373041 [details] [diff] [review] Updated patch for sendBeacon Review of attachment 8373041 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below fixed. And assuming that you've checked (ideally through tests) that we do the right thing when redirects happen wrt 3rd party cookie handling. Yay! ::: dom/base/Navigator.cpp @@ +1134,5 @@ > + > + nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel); > + if (!httpChannel) { > + aRv.Throw(NS_ERROR_DOM_BAD_URI); > + return false; I think sending a beacon to a non-http channel should be allowed. So remove this throwing and add nullchecks below instead. @@ +1148,5 @@ > + nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal(do_QueryInterface(channel)); > + nsCOMPtr<mozIThirdPartyUtil> thirdPartyUtil = do_GetService(THIRDPARTYUTIL_CONTRACTID); > + if (!httpChannelInternal) { > + aRv.Throw(NS_ERROR_DOM_BAD_URI); > + return false; So this whole section should live in an if(httpChannel) block. @@ +1207,5 @@ > + > + nsCOMPtr<nsIUploadChannel2> uploadChannel = do_QueryInterface(channel); > + if (!uploadChannel) { > + aRv.Throw(NS_ERROR_FAILURE); > + return false; Throwing here (as you are doing) is good though. If someone tries to submit a body and the channel doesn't support it we should throw. @@ +1226,5 @@ > + principal, > + true); > + > + // Start a preflight if cross-origin and content type is not whitelisted > + rv = secMan->CheckSameOriginURI(documentURI, uri, true); I think you want the third argument to be 'false'. We shouldn't report an error if the two uris are different origins. @@ +1247,5 @@ > + } else { > + rv = channel->AsyncOpen(cors, nullptr); > + } > + if (NS_FAILED(rv)) { > + aRv.Throw(NS_ERROR_FAILURE); Any reason not to do aRv.Throw(rv)?

Thanks for the quick re-review. Will upload a new patch fixing the last two issues once I get Monica's review. I going to demur on the limitation to the HTTP channel, however, since the Beacon spec currently says explicitly that the UA MUST send the request using the HTTP POST method. As I read it, that implies that the request MUST be HTTP. It seems like we can add support for non-HTTP URIs in a separate increment if this is desired.

Makes sense. But please leave a comment saying why you are throwing.

Comment on attachment 8373041 [details] [diff] [review] Updated patch for sendBeacon Review of attachment 8373041 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks really good, especially for a first patch. f=me for third party checks. ::: dom/base/Navigator.cpp @@ +1044,5 @@ > + > +bool > +Navigator::SendBeacon(const nsAString& aUrl, > + const Nullable<ArrayBufferViewOrBlobOrStringOrFormData>& aData, > + ErrorResult& aRv) This interface seems very strange to me. It is returning both a success boolean and an ErrorResult, but I guess khuey already reviewed the webidl. @@ +1084,5 @@ > + rv = secMan->CheckLoadURIWithPrincipal(principal, > + uri, > + flags); > + if (NS_FAILED(rv)) { > + // Bad URI Why does this not throw? @@ +1099,5 @@ > + nullptr, //extra > + &shouldLoad, > + nsContentUtils::GetContentPolicy(), > + nsContentUtils::GetSecurityManager()); > + if (NS_FAILED(rv)) return false; Why does this not throw? @@ +1115,5 @@ > + return false; > + } > + > + if (csp) { > + channelPolicy = do_CreateInstance("@mozilla.org/nschannelpolicy;1"); NSCHANNELPOLICY_CONTRACTID It's easier to find everyone who uses a particular class if everyone sticks to the named version (and easier to catch typos) @@ +1131,5 @@ > + aRv.Throw(NS_ERROR_FAILURE); > + return false; > + } > + > + nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel); Should this try to pass back the original error? httpChannel = do_QueryInterface(channel, &rv); if (NS_WARN_IF(NS_FAILED(rv))) { // throw rv } and similarly below. https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Use_the_nice_macros @@ +1140,5 @@ > + httpChannel->SetReferrer(documentURI); > + > + // Anything that will need to refer to the window during the request > + // will need to be done now. For example, detection of whether any > + // cookies set by this request are foreign. Note that ThirdPartyUtil Please be specific, nsIThirdPartyUtil.isThirdPartyChannel @@ +1155,5 @@ > + thirdPartyUtil->IsThirdPartyWindow(mWindow, uri, &isForeign); > + httpChannelInternal->SetForceAllowThirdPartyCookie(!isForeign); > + > + nsCString mimeType; > + nsCOMPtr<nsIInputStream> in; You may be able to get away with just using strStream here. Either way is ok. @@ +1156,5 @@ > + httpChannelInternal->SetForceAllowThirdPartyCookie(!isForeign); > + > + nsCString mimeType; > + nsCOMPtr<nsIInputStream> in; > + Please set your editor to highlight tabs and trailing whitespace. @@ +1160,5 @@ > + > + if (!aData.IsNull()) { > + if (aData.Value().IsString()) { > + nsCString stringData = NS_ConvertUTF16toUTF8(aData.Value().GetAsString()); > + nsCOMPtr<nsIStringInputStream> strStream = do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID); All these XPCOM calls need to be checked for failure, either do_CreateInstance(..., &rv) or if (!strStream) @@ +1185,5 @@ > + in = strStream; > + > + } else if (aData.Value().IsBlob()) { > + nsCOMPtr<nsIDOMBlob> blob = aData.Value().GetAsBlob(); > + blob->GetInternalStream(getter_AddRefs(in)); same here, rv = blob->GetInternalStream @@ +1199,5 @@ > + &len, > + mimeType, > + charset); > + } else { > + NS_NOTREACHED("switch statements not in sync"); MOZ_ASSERT should actually crash in debug builds and is probably what you want here: http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#249 ::: dom/tests/mochitest/beacon/beacon-frame.html @@ +11,5 @@ > +{ > + var frame = window.parent.document.getElementById("frame"); > + var data = window.parent.beaconConvert(frame.getAttribute("data")); > + > + navigator.sendBeacon("http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-handler.sjs", data); let result = ... @@ +12,5 @@ > + var frame = window.parent.document.getElementById("frame"); > + var data = window.parent.beaconConvert(frame.getAttribute("data")); > + > + navigator.sendBeacon("http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-handler.sjs", data); > + window.parent.beaconSent(); window.parent.beaconSent(result) ::: dom/tests/mochitest/beacon/beacon-handler.sjs @@ +1,1 @@ > +const CC = Components.Constructor; New file needs standard mozilla header /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ /* vim: set ts=8 sts=2 et sw=2 tw=80: */ * This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ @@ +7,5 @@ > +{ > + // dump(str); > +} > + > +function handleRequest(request, response) { 2-space indents ::: dom/tests/mochitest/beacon/mochitest.ini @@ +1,1 @@ > +[DEFAULT] needs header ::: dom/tests/mochitest/beacon/test_beacon.html @@ +17,5 @@ > +<pre id="test"> > +<script class="testbody" type="text/javascript"> > + > +SimpleTest.waitForExplicitFinish(); > +SpecialPowers.pushPrefEnv({'set': [["beacon.enabled", true]]}, beginTest); var Cr = SpecialPowers.Cr so you can inspect Components.results @@ +23,5 @@ > +function beginTest() { > + > + var threw; > + try { > + navigator.sendBeacon("ftp://example.com", "0"); do_check_eq(false, navigator.sendBeacon()); do_throw("sendBeacon not supported for ftp calls); Actually I don't even think you will get to check the result, you may be able to in the catch block. @@ +25,5 @@ > + var threw; > + try { > + navigator.sendBeacon("ftp://example.com", "0"); > + threw = false; > + } catch (e) { } catch (ex if ex.result == Cr.NS_EXPECTED_ERROR) { } @@ +31,5 @@ > + } > + ok(threw, "sendBeacon not supported for non ftp calls."); > + > + try { > + navigator.sendBeacon(); do_throw("sendBeacon requires more parameters"); @@ +34,5 @@ > + try { > + navigator.sendBeacon(); > + threw = false; > + } catch (e) { > + threw = true; } catch (ex if ex.result == Cr.the error you expect) { } ::: dom/tests/mochitest/beacon/test_beaconContentPolicy.html @@ +1,1 @@ > +<!DOCTYPE HTML> Needs header, and a sentence or 2 describing what this file tests. @@ +29,5 @@ > + > +SpecialPowers.pushPrefEnv({'set': [["beacon.enabled", true]]}, beginTest); > + > +function setupPolicy() { > + var policyID = SpecialPowers.wrap(SpecialPowers.Components).ID("{b80e19d0-878f-d41b-2654-194714a4115c}"); Ouch :( Can you do SpecialPowers.Components.Classes["contract name"] ::: dom/tests/mochitest/beacon/test_beaconCookies.html @@ +17,5 @@ > +<pre id="test"> > +<script class="testbody" type="text/javascript"> > + > +var beaconUrl = "http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-set-cookie.sjs"; > +var beaconUrlAlt = "http://example.org:8888/tests/dom/tests/mochitest/beacon/beacon-set-cookie.sjs"; Is there any way to set the channels that navigator.sendBeacon uses? Otherwise there won't be a way to test if third party checks are correct, like extensions/cookie/test/unit/test_cookies_thirdparty does @@ +31,5 @@ > + SpecialPowers.addObserver(this, "cookie-changed", false); > +} > +examiner.prototype = { > + observe: function examiner_observe(subject, topic, data) { > + window.logEvent(topic); Any reason to pass this handler to the window? @@ +43,5 @@ > + SimpleTest.finish(); > +} > + > +function beginTest() { > + navigator.sendBeacon(beaconUrl, "asdf"); Hmm, seems lacking in imagination ::: dom/tests/mochitest/beacon/test_beaconFrame.html @@ +25,5 @@ > +function getBeaconServerStatus(callback) { > + var request = new XMLHttpRequest(); > + request.open("GET", "http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-handler.sjs?getLastBeacon", true); > + request.onload = function() { > + if (request.readyState === 4) { request.DONE or maybe Ci.nsIXMLHttpRequest.DONE @@ +33,5 @@ > + request.send(null); > +} > + > +function createIframeWithData(data, mimetype, convert) { > + beaconConvert = convert; Does this need to be global? @@ +44,5 @@ > + var c = document.getElementById("content"); > + c.appendChild(frame); > +} > + > +function beaconSent() { How is this function getting called? Nm -- this needs a comment that the beacon-frame calls it after sending the beacon. Btw, it might as well pass back the returned boolean so that the check can be do_check_true(result, "Beacon was sent") ::: dom/webidl/Navigator.webidl @@ +344,5 @@ > NavigatorUserMediaErrorCallback onerror); > }; > #endif // MOZ_MEDIA_NAVIGATOR > + > +partial interface Navigator { This should document what it returns (true on success), when and what it throws, and any other weirdness that the caller should know.

* Addresses comments from Sicking and Chew * Updates WebIDL to match the current ED

Comment on attachment 8373660 [details] [diff] [review] Updated patch for sendBeacon Review of attachment 8373660 [details] [diff] [review]: ----------------------------------------------------------------- I think this has already been checked in, but for next time (in case you need to check it in again). ::: dom/tests/mochitest/beacon/beacon-frame.html @@ +1,1 @@ > +<!DOCTYPE HTML> Still missing header ::: dom/tests/mochitest/beacon/test_beacon.html @@ +1,1 @@ > +<!DOCTYPE HTML> Still missing headers @@ +19,5 @@ > + > +SimpleTest.waitForExplicitFinish(); > +SpecialPowers.pushPrefEnv({'set': [["beacon.enabled", true]]}, beginTest); > + > +function beginTest() { 2-space indent @@ +24,5 @@ > + > + var threw; > + try { > + is(false, navigator.sendBeacon("ftp://example.com", "0")); > + threw = false; Much clearer to actually throw here, I think do_throw("sendBeacon not supported for ftp calls") @@ +25,5 @@ > + var threw; > + try { > + is(false, navigator.sendBeacon("ftp://example.com", "0")); > + threw = false; > + } catch (ex) { And then check that you are catching the error you expect ::: dom/tests/mochitest/beacon/test_beaconContentPolicy.html @@ +4,5 @@ > +https://bugzilla.mozilla.org/show_bug.cgi?id=936340 > +--> > +<head> > + <title>Test for Bug 936340</title> > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> Some trailing whitespace, here and below @@ +28,5 @@ > +var policy = setupPolicy(); > + > +SpecialPowers.pushPrefEnv({'set': [["beacon.enabled", true]]}, beginTest); > + > +function setupPolicy() { 2 space indent ::: dom/tests/mochitest/beacon/test_beaconCookies.html @@ +17,5 @@ > +<pre id="test"> > +<script class="testbody" type="text/javascript"> > + > +var beaconUrl = "http://mochi.test:8888/tests/dom/tests/mochitest/beacon/beacon-set-cookie.sjs"; > +var beaconUrlAlt = "http://example.org:8888/tests/dom/tests/mochitest/beacon/beacon-set-cookie.sjs"; Remove unused variable @@ +39,5 @@ > + > +window.examiner = new examiner(); > + > +function beginTest() { > + navigator.sendBeacon(beaconUrl, "ceci n'est pas une demande"); ha :) ::: dom/tests/mochitest/beacon/test_beaconFrame.html @@ +1,1 @@ > +<!DOCTYPE HTML> Still needs header ::: dom/webidl/Navigator.webidl @@ +344,5 @@ > NavigatorUserMediaErrorCallback onerror); > }; > #endif // MOZ_MEDIA_NAVIGATOR > + > +partial interface Navigator { Please document better

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/6aa771e9aa49 for mochitest-3 failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=34449865&tree=Mozilla-Inbound

(In reply to Wes Kocher (:KWierso) from comment #33) > Backed out in > http://hg.mozilla.org/integration/mozilla-inbound/rev/6aa771e9aa49 for > mochitest-3 failures: > https://tbpl.mozilla.org/php/getParsedLog.php?id=34449865&tree=Mozilla- > Inbound Looks like this also broke mochitest-7 on the b2g ics emulator: https://tbpl.mozilla.org/php/getParsedLog.php?id=34456545&tree=Mozilla-Inbound

Update: Working on figuring out why these tests failed. Updated patch today or Monday.

* Updated WebIDL and re-added Nullable to conform to latest spec * Added some documentation / headers as suggested by mmc * Disabled mochitests on b2g (see Bug 976238) * Try run is pretty much clean (nothing that looks to be our fault) https://tbpl.mozilla.org/?tree=Try&rev=23ab04777cd1

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #37) > https://hg.mozilla.org/integration/mozilla-inbound/rev/0cea4bded6f8 Hi sorry, had to back this out due to test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=35216272&tree=Mozilla-Inbound

Btw, you don't need to write as advanced test assertion messages as you are. Mochitest will print the received and expected value. Something like is(contentType, Ci.nsIContentPolicy.TYPE_BEACON, "Beacon content type should match expected. is: " + contentType + " should be: " + Ci.nsIContentPolicy.TYPE_BEACON); can be simplified to is(contentType, Ci.nsIContentPolicy.TYPE_BEACON, "Beacon content type should match expected.");

Bumped UUID on nsIContentPolicy to ensure that builds pick up the new values.

Backed out because the test_beaconElectrolysis.html file is missing from the patch and therefore it breaks the build: https://hg.mozilla.org/integration/mozilla-inbound/rev/b565f40eea98

(See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8fd72c2a4f4b for the failed build logs)

Removed test_beaconElectrolysis.html from mochitest.ini

This is really exciting news, thanks for the work on this!

Oh, it appears that this landed but is disabled by default. Can someone file a followup to actually enable sendBeacon?

(In reply to Jonas Sicking (:sicking) from comment #48) > Oh, it appears that this landed but is disabled by default. Can someone file > a followup to actually enable sendBeacon? Filed bug 990220.

We'll track the relnote for this in Firefox over on the enabling bug 990220

This is not yet enabled on b2g (see bug 990270)

Thanks!

Docs updated (see bug 990220).