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)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: cpearce, Assigned: jwwang)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
5.71 KB,
patch
|
jwwang
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
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?
Assignee | ||
Comment 6•9 years ago
|
||
Hi Chris, Can you test if the patch works on Windows as well? Thanks.
Reporter | ||
Comment 7•9 years ago
|
||
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-
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
Upload the wrong patch...
Attachment #8587255 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
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)
Reporter | ||
Comment 11•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 12•9 years ago
|
||
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...
Assignee | ||
Comment 13•9 years ago
|
||
rebase.
Attachment #8587257 -
Attachment is obsolete: true
Attachment #8588487 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1e42beda4ee is green.
Keywords: checkin-needed
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ceecfb46d8c
Keywords: checkin-needed
Reporter | ||
Comment 17•9 years ago
|
||
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?
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2ceecfb46d8c
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox39:
--- → affected
Comment 19•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(cpearce)
You need to log in
before you can comment on or make changes to this bug.
Description
•