1234813 - navigator.sendBeacon() throws an exception if blocked by content policies

Status: RESOLVED FIXED

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

Description

[Wladimir Palant]

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.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1234813_sendbeacon_tests.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1234813_sendbeacon.patch

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

Comment 3

[Richard Barnes [:rbarnes]]

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

Comment 4

[Richard Barnes [:rbarnes]]

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.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(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?

Comment 6

[Richard Barnes [:rbarnes]]

Ah yes, I just forgot what NS_ENSURE_SUCCESS meant.

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

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Thanks Richard - this is ready to be landed!

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Pulsebot]

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

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1234813_sendbeacon_webplatformtest_updates.patch

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

Comment 13

[James Graham [:jgraham]]

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.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 15

[Richard Barnes [:rbarnes]]

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?

Comment 16

[James Graham [:jgraham]]

(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.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

[Eric Shepherd [:sheppy]]

Documentation updated:

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