Change behavior of subdocument Flash blocking to be third-party Flash blocking

RESOLVED FIXED in Firefox 53

Status

()

Core
Plug-ins
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: bytesized, Assigned: bytesized)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
List-based Flash Blocking (Bug 1307604) currently uses a list to block Flash in subdocuments, but it should instead block Flash in third-party subdocuments only.

Subdocuments that will now be exempted from the third-party Flash block list can be described as follows:

 - The subdocument must have the exact same domain, subdomain and scheme (http/https) as the top level document.
 - The subdocument must not have been loaded within a third-party subdocument.
Yep sorry again for missing that detail!
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8846066 - Flags: review?(benjamin)
(Assignee)

Updated

3 months ago
Attachment #8846066 - Flags: review?(kyle)

Comment 3

3 months ago
mozreview-review
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121150

::: toolkit/components/url-classifier/flash-block-lists.rst:45
(Diff revision 1)
> +Third-Party Documents
> +=====================
> +
> +For a document to be considered a Third-Party document, it must not be a top-level document, and must meet AT LEAST ONE of these requirements:
> +
> +* The document's parent is a Third-Party document.

These rules describe what "third-party" means in a roundabout way. Rather than hand-coding a definition of third-party, we should be using standard same-origin checks which already do all of this for free and probably catch the edge cases.

nsIPrincipal->Equals(parent)
Attachment #8846066 - Flags: review?(benjamin) → review-

Comment 4

3 months ago
mozreview-review
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121152

::: commit-message-800ba:1
(Diff revision 1)
> +Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

Also, this patch appears to not have any tests.
(Assignee)

Comment 5

3 months ago
mozreview-review-reply
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121152

> Also, this patch appears to not have any tests.

There is an existing Flash Blocking test (toolkit/components/url-classifier/tests/browser/browser_flash_block_lists.js), which I have modified to verify that Flash blocking works as expected for third-party and non-third-party subdocuments. The test modifications are included in this patch. Did you want additional testing beyond this?
(Assignee)

Updated

3 months ago
Flags: needinfo?(benjamin)
(Assignee)

Comment 6

3 months ago
mozreview-review-reply
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121150

> These rules describe what "third-party" means in a roundabout way. Rather than hand-coding a definition of third-party, we should be using standard same-origin checks which already do all of this for free and probably catch the edge cases.
> 
> nsIPrincipal->Equals(parent)

I will change the code to use `nsIPrincipal::Equals`, but it sounds like you are also unhappy with my documentation for the desired functionality. I thought that I summarized our conversation regarding this well. Could you tell me how you would like the documentation changed?
I think that rather than redefining third-party, this should just say "subdocument which is not same-origin" and assume the web-standards definition of a same-origin frame.
Flags: needinfo?(benjamin)
(Assignee)

Comment 8

3 months ago
What about the additional testing (Comment 5)?
Flags: needinfo?(benjamin)
Is there any automated testing for this feature? If so those tests should be updated to reflect this change in the policy. Otherwise this really needs some mochitests.
Flags: needinfo?(benjamin)
(Assignee)

Comment 10

3 months ago
As I said in Comment 5, there is an automated test for this feature located at toolkit/components/url-classifier/tests/browser/browser_flash_block_lists.js which this patch modifies to reflect this change in policy.

Is additional testing, other than what I have mentioned, needed here?
Flags: needinfo?(benjamin)
Oh, I'm sorry I somehow missed that. I was looking for a mochitest. Yes that looks like it covers what you need.
Flags: needinfo?(benjamin)
Comment hidden (mozreview-request)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review121994

r=me but I'm not a DOM peer so qdot or bz also needs to review this
Attachment #8846066 - Flags: review?(benjamin) → review+
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

https://reviewboard.mozilla.org/r/119150/#review122194

LGTM
Attachment #8846066 - Flags: review?(kyle) → review+

Comment 15

2 months ago
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3defd1bf850f
Change behavior of subdocument Flash blocking to be Third-Party Flash blocking r=bsmedberg,qdot
(Assignee)

Comment 16

2 months ago
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Study's handling of third-party domains will be incorrect
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: This changes the behavior of code that is currently pref-ed off and will only be exposed to users who are part of the study.
[String changes made/needed]: none
Attachment #8846066 - Flags: approval-mozilla-beta?
Attachment #8846066 - Flags: approval-mozilla-aurora?

Comment 17

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3defd1bf850f
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8846066 [details]
Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking

For shield study purpose of making flash click-to-play by default. Beta53+ & Aurora54+.
Attachment #8846066 - Flags: approval-mozilla-beta?
Attachment #8846066 - Flags: approval-mozilla-beta+
Attachment #8846066 - Flags: approval-mozilla-aurora?
Attachment #8846066 - Flags: approval-mozilla-aurora+

Updated

2 months ago
status-firefox53: --- → affected
status-firefox54: --- → affected

Comment 19

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0383139bd7b
status-firefox54: affected → fixed
has problems on beta , so maybe need a rebase or so 

grafting 405310:d0383139bd7b "Bug 1345611 - Change behavior of subdocument Flash blocking to be Third-Party Flash blocking r=bsmedberg,qdot a=gchang"
merging dom/base/nsDocument.cpp
merging dom/base/nsDocument.h
merging dom/base/nsIDocument.h
merging toolkit/components/url-classifier/tests/browser/browser_flash_block_lists.js
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsDocument.h! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/base/nsIDocument.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(ksteuber)
(Assignee)

Comment 21

2 months ago
Ah. It seems that I requested that Bug 1338287 be uplifted to 52 (which was denied), but not 53. I will request uplift on that bug first.
Flags: needinfo?(ksteuber)
(In reply to Kirk Steuber [:bytesized] from comment #21)
> Ah. It seems that I requested that Bug 1338287 be uplifted to 52 (which was
> denied), but not 53. I will request uplift on that bug first.

Hey Kirk, this actually did the trick :) worked after the other bug landed :)

Comment 23

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/993b574eb828
status-firefox53: affected → fixed
You need to log in before you can comment on or make changes to this bug.