Closed
Bug 1345611
Opened 8 years ago
Closed 8 years ago
Change behavior of subdocument Flash blocking to be third-party Flash blocking
Categories
(Core Graveyard :: Plug-ins, defect, P1)
Core Graveyard
Plug-ins
Tracking
(firefox53 fixed, firefox54 fixed, firefox55 fixed)
RESOLVED
FIXED
mozilla55
People
(Reporter: bytesized, Assigned: bytesized)
References
Details
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
benjamin
:
review+
qdot
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8846066 -
Flags: review?(benjamin)
Assignee | ||
Updated•8 years ago
|
Attachment #8846066 -
Flags: review?(kyle)
Comment 3•8 years 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•8 years 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•8 years 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•8 years ago
|
Flags: needinfo?(benjamin)
Assignee | ||
Comment 6•8 years 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?
Comment 7•8 years ago
|
||
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•8 years ago
|
||
What about the additional testing (Comment 5)?
Flags: needinfo?(benjamin)
Comment 9•8 years ago
|
||
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•8 years 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)
Comment 11•8 years ago
|
||
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•8 years 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 14•8 years 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/#review122194
LGTM
Attachment #8846066 -
Flags: review?(kyle) → review+
Comment 15•8 years 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•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 18•8 years ago
|
||
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•8 years ago
|
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Comment 19•8 years ago
|
||
bugherder uplift |
Comment 20•8 years ago
|
||
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•8 years 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)
Comment 22•8 years ago
|
||
(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•8 years ago
|
||
bugherder uplift |
Comment 24•6 years ago
|
||
I'm porting the flash-classifier to a necko layer and I wonder why we have nsIDocument::IsThirdParty() instead of using mozIThirdPartyUtil::IsThirdPartyWindow/IsThirdPartyChannel().
I'm asking that because they produce 2 different results: mozIThirdPartyUtil considers "first-party" if URLs have the same base-domain, but nsIDocument::IsThirdParty() doesn't. This means that: http://subdomain.example.com is a first-party of http://example.com with the former, but it's not with the latter method.
Do we have any particular constrains related to the flash-classifier to have this nsIDocument::IsThirdParty() or we can unify our code base and use mozIThirdPartyUtil?
Flags: needinfo?(ksteuber)
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•