Closed Bug 1150277 Opened 9 years ago Closed 9 years ago

[EME] "Forget about this site" does not clear GMP storage

Categories

(Core :: Audio/Video, defect, P1)

39 Branch
x86_64
Windows 8.1
defect

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: cpearce, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

STR (Firefox win32 builds only):
1. Open http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/
2. Press "Play" button on page.
3. In Windows Explorer, open %appdata%/Mozilla/Firefox/Profiles/$profile/gmp/storage and note the folder containing files with the latest timestamp changed; that'll be for "drmtest2.adobe.com".
4. Back in Firefox, press CTRL+H to open history.
5. Search for "drmtest2.adobe.com"
6. Right click on result, select "forget about this site".
7. Back in Windows Explorer, note that the folder previously identified is unchanged.

I thought we were supposed to kill active plugins when we cleared storage?
JW: Can you look into this bug please?
Flags: needinfo?(jwwang)
Sure.
Assignee: nobody → jwwang
Flags: needinfo?(jwwang)
I can repro this bug on Linux by opening http://people.mozilla.org/~cpearce/mse-clearkey/.

Here are some logs:

MediaKeys[7f0da4bd2800]::Create() (http://people.mozilla.org, http://people.mozilla.org), NonPrivateBrowsing
The origin string returned by |nsContentUtils::GetUTFOrigin(mPrincipal, origin)| in MediaKeys::Init() is "http://people.mozilla.org".

GMPService::ForgetThisSiteOnGMPThread: origin=people.mozilla.org
However, the origin string passed to mozIGeckoMediaPluginService.forgetThisSite() is "people.mozilla.org".

Therefore, the strings do not match and the data is not cleared.
Attached patch 1150277_match_hostname-v1.patch (obsolete) — Splinter Review
ForgetThisSite pass a hostname instead of an origin. We should clear the data when the origin contains the hostname instead of matching it exactly.
Attachment #8587163 - Flags: review?(cpearce)
Comment on attachment 8587163 [details] [diff] [review]
1150277_match_hostname-v1.patch

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

::: dom/media/gmp/GMPService.cpp
@@ +1332,5 @@
>  
>    nsresult rv;
>    nsCString str;
>    rv = ReadFromFile(aPath, NS_LITERAL_CSTRING("origin"), str, MaxDomainLength);
> +  if (NS_SUCCEEDED(rv) && str.Find(aSite.Data()) != -1) {

Is there a way to extract the hostname from an origin so that we can match hostnames without searching for substring?
Hi Chris,
Can you test if the patch works on Windows as well? Thanks.
Comment on attachment 8587163 [details] [diff] [review]
1150277_match_hostname-v1.patch

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

I'll r- this because I think we should do an exact match against the origins we've stored in $profiledir/gmp/id/$nodeId/origin and topLevelOrigin. Hopefully nsIEffectiveTLDService is what you need here...

::: dom/media/gmp/GMPService.cpp
@@ +1332,5 @@
>  
>    nsresult rv;
>    nsCString str;
>    rv = ReadFromFile(aPath, NS_LITERAL_CSTRING("origin"), str, MaxDomainLength);
> +  if (NS_SUCCEEDED(rv) && str.Find(aSite.Data()) != -1) {

I think nsIEffectiveTLDService::GetBaseDomainFromHost would do this.

We should instead extract the base domain from |str| here and below, and compare against that. Then we're doing an exact match. i.e. strip of the "http://" prefix.

Also, note that since aSite is an abstract nsACString (rather than an nsCString or other concrete string class), you are not guaranteed that its .Data() buffer is null terminated. You could be being passed an abstract class that is a substring of another string for example. So you shouldn't use nsACString::Data() where a null terminated c-style string is expected.

Some of our logging in GMPService calls .Data() and expects it to be null terminated... I only just learned about this recently. :(
Attachment #8587163 - Flags: review?(cpearce) → review-
Attached patch 1150277_match_hostname-v2.patch (obsolete) — Splinter Review
nsIEffectiveTLDService functions can only be called on the main thread. I will invent the wheel again to strip scheme and port from an origin string.
Attachment #8587163 - Attachment is obsolete: true
Attached patch 1150277_match_hostname-v3.patch (obsolete) — Splinter Review
Upload the wrong patch...
Attachment #8587255 - Attachment is obsolete: true
Comment on attachment 8587257 [details] [diff] [review]
1150277_match_hostname-v3.patch

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

interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8587163&action=interdiff&newid=8587257&headers=1

nsIEffectiveTLDService functions can only be called on the main thread. I will invent the wheel again to strip scheme and port from an origin string.
Attachment #8587257 - Flags: review?(cpearce)
Comment on attachment 8587257 [details] [diff] [review]
1150277_match_hostname-v3.patch

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

Great, thanks. Maybe "host" is a more accurate term to use than "site".

We should really get a test for this...
Attachment #8587257 - Flags: review?(cpearce) → review+
Flags: in-testsuite?
We did. https://hg.mozilla.org/mozilla-central/file/da2f28836843/dom/media/gtest/TestGMPCrossOrigin.cpp#l492
Alas our origin strings in the test do not include scheme nor port number...
rebase.
Attachment #8587257 - Attachment is obsolete: true
Attachment #8588487 - Flags: review+
Must uplift...
Flags: needinfo?(cpearce)
Comment on attachment 8588487 [details] [diff] [review]
1150277_match_hostname-v4.patch

Approval Request Comment
[Feature/regressing bug #]: EME + "Forget about this site"
[User impact if declined]: Users that opt to "Forget about this site" from the history sidebar won't clear data written by EME plugins, leaking information.
[Describe test coverage new/current, TreeHerder]: Local testing. We have existing tests that cover this feature, but not this specific case.
[Risks and why]: Pretty low, we have tests that cover this feature in general.
[String/UUID change made/needed]: None.
Attachment #8588487 - Flags: approval-mozilla-beta?
Attachment #8588487 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2ceecfb46d8c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Blocks: 1151746
Comment on attachment 8588487 [details] [diff] [review]
1150277_match_hostname-v4.patch

Should be in 38 beta 3
Attachment #8588487 - Flags: approval-mozilla-beta?
Attachment #8588487 - Flags: approval-mozilla-beta+
Attachment #8588487 - Flags: approval-mozilla-aurora?
Attachment #8588487 - Flags: approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: