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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: gaubugzilla, Assigned: ckerschb)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 affected, firefox50 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

4 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.
Flags: needinfo?(rlb)
Flags: needinfo?(mozilla)
Flags: needinfo?(mozilla)
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
Attachment #8701579 - Flags: review?(rlb)
Flags: needinfo?(rlb)
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 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.
Attachment #8701577 - Flags: feedback+
Attachment #8701579 - Flags: review?(rlb) → review-
(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?
Flags: needinfo?(rlb)
Ah yes, I just forgot what NS_ENSURE_SUCCESS meant.

Maybe update the comment to say "Return false, but do not throw, if..."
Flags: needinfo?(rlb)
Attachment #8701579 - Flags: review- → review+
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.
Attachment #8701577 - Flags: review?(rlb)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8701577 - Flags: review?(rlb) → review+
Thanks Richard - this is ready to be landed!
Keywords: checkin-needed
James, given comment 0, I suppose this test is wrong.
Any chance I can land an update for the test on mozilla-central?
Flags: needinfo?(mozilla)
Attachment #8718529 - Flags: review?(james)
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.
(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.
Flags: needinfo?(rlb)
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?
Flags: needinfo?(rlb)
(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.
Attachment #8718529 - Flags: review?(james) → review+
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

3 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

Comment 19

3 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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.