Wladimir Palant Reporter
	Description • 8 years ago

Steps to reproduce:

1. Install Adblock Plus from https://addons.mozilla.org/addon/adblock-plus/
2. Open Web Console on some webpage (e.g. by pressing Cmd-Alt-K on OS X).
3. Enter the following code and press Enter:

  navigator.sendBeacon("/microad.foo");

This request will be blocked by the filter "/microad." in EasyList (the filter list which will normally be activated in Adblock Plus by default).

Expected results:

The call succeeds and returns true - navigator.sendBeacon() queues the request but doesn't guarantee that it will succeed.

Actual results:

The call throws an exception with error code 0x805e0006 (NS_ERROR_CONTENT_BLOCKED). An exception isn't something that the web code expects here - documentation doesn't indicate that this call could ever throw.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 2 • 8 years ago

Richard, it seems that the content policy checks were already throwing an exception even before we converted sendBeacon to use asyncOpen2 [1]. So, not sure why this problem only seems to appear now and not before.

In case we don't want to throw if content policy checks are failing then this would be a potential patch (and testcase). Probably we even have to remove more of the 'aRv.Throw(rv)' from within sendBeacon.

Not really sure if this is even a review request, let me know what you think!

[1] http://hg.mozilla.org/mozilla-central/rev/a11078290110#l1.134

	Richard Barnes [:rbarnes]
	Comment 3 • 8 years ago

ckerschb: This probably would have been a good use of "feedback?" :)

It's not totally clear to me that this is actually a bug.

On the one hand, the sendBeacon spec [1] calls through to Fetch [2], which of course can throw a wide variety of exceptions.  (Since this error is coming from asyncOpen2, it's effectively coming from fetch.)

On the other hand, the call to fetch occurs after the spec says to "return the sendBeacon() call, but continue to runs [sic] the following steps".  So I could see how you could expect the error to go outside the page context.

Net of all that hand wringing, I suggest we simply return false.  The sendBeacon spec says that the method returns true iff "if the user agent is able to successfully queue the data for transfer".  In this case, the data has not been queued for transfer, in particular because it was blocked for security reasons.  So just delete the throw line.

[1] http://www.w3.org/TR/beacon/#sec-processing-model
[2] https://fetch.spec.whatwg.org/#concept-main-fetch

	Richard Barnes [:rbarnes]
	Comment 4 • 8 years ago

Comment on attachment 8701577 [details] [diff] [review]
bug_1234813_sendbeacon_tests.patch

Review of attachment 8701577 [details] [diff] [review]:
-----------------------------------------------------------------

This looks generally correct, though if we agree that sendBeacon should return false, then we should test that.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 5 • 8 years ago

(In reply to Richard Barnes [:rbarnes] from comment #3)
> Net of all that hand wringing, I suggest we simply return false.  The
> sendBeacon spec says that the method returns true iff "if the user agent is
> able to successfully queue the data for transfer".  In this case, the data
> has not been queued for transfer, in particular because it was blocked for
> security reasons.  So just delete the throw line.

Hey Richard, I agree, we should just return false and remove the throw line, but isn't that exactly what my patch is proposing?

	Richard Barnes [:rbarnes]
	Comment 6 • 8 years ago

Ah yes, I just forgot what NS_ENSURE_SUCCESS meant.

Maybe update the comment to say "Return false, but do not throw, if..."

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 7 • 8 years ago

Comment on attachment 8701577 [details] [diff] [review]
bug_1234813_sendbeacon_tests.patch

Richard, sorry I somewhat lost this bug on my radar, since it wasn't assigned to me. Now, after that we have agreed on the patch, I also think that the test is what we want in the end. I hope you agree and we can get those patches landed.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 8 • 8 years ago

Thanks Richard - this is ready to be landed!

	Pulsebot
	Comment 9 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9aecc2e6334
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b8eef80b22

	Carsten Book [:Tomcat]
	Comment 10 • 8 years ago

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=21511751&repo=mozilla-inbound

	Pulsebot
	Comment 11 • 8 years ago

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47f2ca683d37
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea9d3fffab46

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 12 • 8 years ago

James, given comment 0, I suppose this test is wrong.
Any chance I can land an update for the test on mozilla-central?

	James Graham [:jgraham]
	Comment 13 • 8 years ago

Comment on attachment 8718529 [details] [diff] [review]
bug_1234813_sendbeacon_webplatformtest_updates.patch

Are we clear that there is general agreement on our interpretation of the spec here? In particular given this is a test from Google, I wonder if they agree on the expected behaviour in this case.

Further, if we are returning a value from sendBeacon we should really check that we get the expected value rather than just using "did not throw" as the pas condition.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 14 • 8 years ago

(In reply to James Graham [:jgraham] from comment #13)
> Comment on attachment 8718529 [details] [diff] [review]
> bug_1234813_sendbeacon_webplatformtest_updates.patch
> 
> Are we clear that there is general agreement on our interpretation of the
> spec here? In particular given this is a test from Google, I wonder if they
> agree on the expected behaviour in this case.

Well, it's debatable. Since Richard implemented most of the feature I would like to hear his opinion again before landing the update. I personally think we should land the patch, at least that's how I interpret the spec. Richard, what do you think?

> Further, if we are returning a value from sendBeacon we should really check
> that we get the expected value rather than just using "did not throw" as the
> pas condition.

Sure, once I hear back from Richard we can update that part of the test of course.

	Richard Barnes [:rbarnes]
	Comment 15 • 8 years ago

As I said above, I think it's in line with the spec to return false and not throw in this case.

That said, it would be good to create a test case and check to see what Chrome does in this case?

	James Graham [:jgraham]
	Comment 16 • 8 years ago

(In reply to Richard Barnes [:rbarnes] from comment #15)
> That said, it would be good to create a test case and check to see what
> Chrome does in this case?

Please do.

	Christoph Kerschbaumer [:ckerschb] Assignee
	Comment 17 • 8 years ago

The changes within this bug actually never landed, but given Richard's response and my interpretation of the spec I think we are doing the right thing here. Rebased the patches, let's see how we are doing on TRY and then let's land those changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d67a593b8f4e

	Pulsebot
	Comment 18 • 8 years ago

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f8a3181ab8f
sendBeacon should not throw if blocked by Content Policy. r=rbarnes
https://hg.mozilla.org/integration/mozilla-inbound/rev/112d5a8b9f4e
Tests for: sendBeacon should not throw if blocked by Content Policy. r=barnes
https://hg.mozilla.org/integration/mozilla-inbound/rev/a81af903aac1
WPT Tests update: sendBeacon should not throw if blocked by content policies. r=james

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1f8a3181ab8f
https://hg.mozilla.org/mozilla-central/rev/112d5a8b9f4e
https://hg.mozilla.org/mozilla-central/rev/a81af903aac1

	Eric Shepherd [:sheppy]
	Comment 20 • 8 years ago

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon
https://developer.mozilla.org/en-US/Firefox/Releases/50