Closed
Bug 1129040
Opened 9 years ago
Closed 9 years ago
[e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm
Categories
(Toolkit :: Add-ons Manager, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: Gijs, Assigned: jimm)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 23 obsolete files)
8.98 KB,
patch
|
Details | Diff | Splinter Review | |
9.61 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
3.92 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
2.61 KB,
patch
|
billm
:
review+
|
Details | Diff | Splinter Review |
279.19 KB,
patch
|
Details | Diff | Splinter Review |
Clean profile on current fx-team tip, shows: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get] FileUtils.jsm:63:0 as soon as you show a web-based page (the mozilla update/welcome page that shows when creating a new profile will trigger this, but otherwise http://www.apple.com/it/itunes/download/ will do it) which is: var dir = gDirService.get(key, Ci.nsIFile); in FileUtils.getDir. On the second startup, you can trigger it by restoring the session from about:home (which itself doesn't trigger this), and catch it in the browser content toolbox if you open that before restoring (you'll need to Components.utils.import("resource://gre/modules/FileUtils.jsm") from the console in order to set a breakpoint there. Then you'll see that it's triggered by the plugin blocklist service. Georg, can you investigate?
Flags: needinfo?(gfritzsche)
Comment 1•9 years ago
|
||
I don't really have time to investigate this at the moment. Do you know what the impact of this is? Is this breaking plugin blocks? Do you have a stack handy that shows what components are involved?
Flags: needinfo?(gfritzsche) → needinfo?(gijskruitbosch+bugs)
Comment 2•9 years ago
|
||
Is this with e10s? AFAIK the blocklist service, if it is used at all in the content process, should be getting its data from the chrome process. Perhaps that's the bug here.
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > Is this with e10s? Ugh @ me for leaving out key information. Yes, this is with e10s (although the content toolbox process maybe gave it away, I should have been explicit) I don't have a stack handy because the js debugger doesn't make it easily copy-able. :-( I can look into it again tomorrow, I've moved on now and am rushing to get some more urgent things done today. I don't know if/what it breaks because I actually don't know what the plugin blocklist service *is*, I just know that that's what I saw on the stack so that's where I moved this.
Reporter | ||
Comment 4•9 years ago
|
||
So this might require you to have a plugin globally installed? I have Adobe Acrobat Reader on my machine, and so it seems that: nsBlockListService.js' /* See nsIBlocklistService */ getPluginBlocklistState: function Blocklist_getPluginBlocklistState(plugin, appVersion, toolkitVersion) { if (!this._isBlocklistLoaded()) this._loadBlocklist(); return this._getPluginBlocklistState(plugin, this._pluginEntries, appVersion, toolkitVersion); }, gets called with plugin having .description "Adobe PDF Plug-In For Firefox and Netscape 11.0.10" (filename nppdf32.dll) which calls _loadBlockList, which wants to get the profile dir, which calls FileUtils.getFile, which calls FileUtils.getDir, which throws. So I guess that comment #2 is right and this needs to be reimplemented so it gets the blocklist data from the chrome process? It looks like this code wants an answer synchronously, though, so that might be interesting...
Flags: needinfo?(gijskruitbosch+bugs)
Summary: Plugin block list triggers exception in FileUtils.jsm getDIr, for nsIProperties.get passing "ProfD" and nsIFile → [e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm getDIr, for the directory service's get method passing "ProfD" and nsIFile
Assignee | ||
Updated•9 years ago
|
Updated•9 years ago
|
Component: Plug-ins → Add-ons Manager
Product: Core → Toolkit
Comment 5•9 years ago
|
||
Thanks for checking Gijs :)
Assignee | ||
Updated•9 years ago
|
Blocks: e10s-plugins
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jmathies
Assignee | ||
Comment 6•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1fc5191f202
Assignee | ||
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=090ff8636230
Assignee | ||
Updated•9 years ago
|
Summary: [e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm getDIr, for the directory service's get method passing "ProfD" and nsIFile → [e10s] Plugin block list tries to load blocklist and triggers exception in FileUtils.jsm
Assignee | ||
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=090ff8636230 Hey Kamil, would you mind trying to reproduce bug 1130165 using this try build when you have a chance?
Flags: needinfo?(kjozwiak)
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #8) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=090ff8636230 Mostly green, the leaks are probably caused by a lack of unregistration of the factory on shutdown.
Assignee | ||
Comment 12•9 years ago
|
||
Dave, I think this is in your neck of the woods. Feel free to reassign.
Attachment #8576834 -
Attachment is obsolete: true
Attachment #8577310 -
Flags: review?(dtownsend)
Comment 13•9 years ago
|
||
Comment on attachment 8577310 [details] [diff] [review] patch Review of attachment 8577310 [details] [diff] [review]: ----------------------------------------------------------------- Some mxr searches tell me that the only method we actually need to care about child processes being able to call is getPluginBlocklistState so I think that is all we should implement in the child. The rest either don't implement or just have them throw an exception. Defining this in browser-child is the wrong place, as a frame script that will be run many times in the same process and so attempt to recreate a new blocklist service each time. Instead we should just define a version of the blocklist service that only loads in the child process, nsBlocklistServiceChild.js, and have that send messages to nsBlocklistService.js. We probably don't need to send the plugin tag across as a CPOW, all the service cares about is a few string properties on it so you could just copy those to a plain JS object. All the browser-chrome plugin tests are disabled in e10s right now. Some of them do blocklist checking so it would be interesting to see if any can be enabled: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser.ini
Attachment #8577310 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #13) > Comment on attachment 8577310 [details] [diff] [review] > patch > > Review of attachment 8577310 [details] [diff] [review]: > ----------------------------------------------------------------- > > Some mxr searches tell me that the only method we actually need to care > about child processes being able to call is getPluginBlocklistState so I > think that is all we should implement in the child. The rest either don't > implement or just have them throw an exception. Ok, no problem. I was wondering about this as well, I ended up going the other way and implementing these. Will remove that and just error out. > Defining this in browser-child is the wrong place, as a frame script that > will be run many times in the same process and so attempt to recreate a new > blocklist service each time. Instead we should just define a version of the > blocklist service that only loads in the child process, > nsBlocklistServiceChild.js, and have that send messages to > nsBlocklistService.js. I think I had browser-child.js mixed up with broiwser-content.js, I was going for load once behavior. Will look into the separate module. > We probably don't need to send the plugin tag across as a CPOW, all the > service cares about is a few string properties on it so you could just copy > those to a plain JS object. That's what's I'm doing with flattenObject. > All the browser-chrome plugin tests are disabled in e10s right now. Some of > them do blocklist checking so it would be interesting to see if any can be > enabled: > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/ > plugins/browser.ini Thanks for the reminder.
Comment 15•9 years ago
|
||
Used the following m-c build for verification: * http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-090ff8636230/ ** http://download.macromedia.com/pub/flashplayer/installers/archive/fp_16.0.0.296_archive.zip ** http://download.macromedia.com/pub/flashplayer/installers/archive/fp_11.2.202.440_archive.zip Went through the following test cases while e10s was enabled/disabled: - visited ign.com and ensured that the vulnerable plugin is being blocked - ensured that flash appears as vulnerable under about:addons - ensured that selecting "Activate Flash" works correctly and plays the video - selected "Allow Now" and ensured that when fx is restarted, the video is still blocked - selected "Allow and Remember" and ensured that the site isn't blocked when fx is restarted OS's: - Win 8.1 x64 - PASSED - Ubuntu 14.04.1 x64 - PASSED Looks like it's working Jim, once this lands into m-c I'll do more thorough testing with different versions of vulnerable flash versions. Let me know if there's anything else that you need!
Flags: needinfo?(kjozwiak)
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Kamil Jozwiak [:kjozwiak] from comment #15) > OS's: > > - Win 8.1 x64 - PASSED > - Ubuntu 14.04.1 x64 - PASSED > > Looks like it's working Jim, once this lands into m-c I'll do more thorough > testing with different versions of vulnerable flash versions. Let me know if > there's anything else that you need! Great, thanks!
Assignee | ||
Updated•9 years ago
|
Attachment #8577310 -
Attachment is obsolete: true
Assignee | ||
Comment 17•9 years ago
|
||
Assignee | ||
Comment 18•9 years ago
|
||
Comment on attachment 8578938 [details] [diff] [review] patch This is the core blocklist patch. Per discussion in triage, I will file a follow up on updating tests in: browser/base/content/tests/plugins/*
Attachment #8578938 -
Flags: review?(dtownsend)
Comment 19•9 years ago
|
||
Comment on attachment 8578938 [details] [diff] [review] patch Review of attachment 8578938 [details] [diff] [review]: ----------------------------------------------------------------- A few ways this can be simplified otherwise this looks great. ::: toolkit/mozapps/extensions/extensions.manifest @@ +3,5 @@ > +category profile-after-change nsBlocklistService @mozilla.org/extensions/blocklist;1 process=main > + > +component {e0a106ed-6ad4-47a4-b6af-2f1c8aa4712d} nsBlocklistServiceContent.js process=content > +contract @mozilla.org/extensions/blocklist;1 {e0a106ed-6ad4-47a4-b6af-2f1c8aa4712d} process=content > +category app-startup nsBlocklistService @mozilla.org/extensions/blocklist;1 process=content I don't think we need this line. If something tries to use the blocklist service it will load and work but otherwise there is no need to load this on startup. ::: toolkit/mozapps/extensions/nsBlocklistService.js @@ +298,5 @@ > gPref.addObserver(PREF_EM_LOGGING_ENABLED, this, false); > this.wrappedJSObject = this; > + // requests from child processes come in here, see receiveMessage. > + this._ppmm = Cc['@mozilla.org/parentprocessmessagemanager;1']. > + getService(Ci.nsIMessageBroadcaster); Just use Services.ppmm instead of keeping our own reference. @@ +371,5 @@ > + default: > + throw new Error("Unknown blocklist message received from content: " + aMsg.name); > + } > + }, > + Nit: whitespace at end of line ::: toolkit/mozapps/extensions/nsBlocklistServiceContent.js @@ +30,5 @@ > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsIBlocklistService]), > + > + init: function () { > + this._mm = Cc['@mozilla.org/childprocessmessagemanager;1']. > + getService(Ci.nsISyncMessageSender); Rather than retaining this just use Services.cpmm. @@ +34,5 @@ > + getService(Ci.nsISyncMessageSender); > + Services.obs.addObserver(this, "xpcom-shutdown", false); > + }, > + > + uninit: function () { We're not doing anything here so no reason to listen for xpcom-shutdown or have this be an nsIObserver. That probably makes the init method unnecessary too.
Attachment #8578938 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 20•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dafab239bb15
Attachment #8578938 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
right patch this time.
Attachment #8581598 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cee8f9d83b7e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cee8f9d83b7e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 24•9 years ago
|
||
Jim, looks like the flash blocklisting is still not working in e10s. I downloaded fp_16.0.0.296 which is blocklisted but flash video's are still being played automatically even though the plugin is set to "Ask to Activate". The original try build from comment # 10 works as expected and blocked the flash video in both e10s and none-e10s modes. However with the latest nightly, the flash video will play automatically start playing on e10s despite being set as "Ask to Activate". If you switch over to none-e10s, it behaves correctly. STR: * installed fp_16.0.0.296 * downloaded and installed the latest nightly (build linked below) * opened nightly and visited ign.com and played a random video which played automatically * disabled e10s through the Options and re-started the browser * same video was blocked with the "Plugin is vulnerable" error message Builds Used: Working -> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-090ff8636230/ Not Working -> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-03-26-03-02-12-mozilla-central/ Win 8.1 x64 -> Reproduced using fp_16.0.0.296 Ubuntu 14.04.1 -> Reproduced using 11.2.202.440
Flags: needinfo?(jmathies)
Assignee | ||
Updated•9 years ago
|
Status: RESOLVED → REOPENED
Flags: needinfo?(jmathies)
Resolution: FIXED → ---
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Kamil Jozwiak [:kjozwiak] from comment #24) > Jim, looks like the flash blocklisting is still not working in e10s. Thanks for checking back Kamil!
Depends on: 1148776
Assignee | ||
Updated•9 years ago
|
Attachment #8581601 -
Attachment description: patch r=Mossop → patch r=Mossop [checked in]
Assignee | ||
Comment 26•9 years ago
|
||
We cache blocklist values in plugin tags, unless we receive a blocklist-updated observer.. in which case the next time we need the blocklist value, we ask the service. This propagates that observer over to the content process so its tags get invalidated as well.
Assignee | ||
Updated•9 years ago
|
Attachment #8585077 -
Flags: review?(dtownsend)
Assignee | ||
Comment 27•9 years ago
|
||
If the blocklist forwarding service in the content process gets highjacked or breaks, we need to check directly with chrome at least once (I'm doing it twice) before instantiating a plugin instance. Basically we shouldn't trust that the nsBlocklistService in the content process is right.
Assignee | ||
Comment 28•9 years ago
|
||
tighten this up a bit. We want most of this interface to fail so that its use is noticed.
Assignee | ||
Comment 29•9 years ago
|
||
Assignee | ||
Comment 30•9 years ago
|
||
Still working on getting all these tests functional. Currently dealing with some linux failures on try which do not reproduce locally. Also I'm getting a mochitest-plain 3 failure which seems to be caused by something in these patches, but I'm not sure what it is. The tests can't reach the plugin native apis. I ran into this in the tests I actually had to fix, the key was to get rid of xray vision on the object. I'm not sure how my patches here would cause a failure of that in tests I haven't touched. :) Need to keep digging. https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc51523e4dc5
Assignee | ||
Updated•9 years ago
|
Comment 32•9 years ago
|
||
Comment on attachment 8585077 [details] [diff] [review] fix for blocklist updates v.1 Review of attachment 8585077 [details] [diff] [review]: ----------------------------------------------------------------- I think we have a few cases where the parent just wants to forward observer notifications to the child. I wonder if at some point we might want a more generic way to do that. Oh well, something for the future.
Attachment #8585077 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 33•9 years ago
|
||
Latest wip, hopefully with the odd plugin related test failures fixed - https://treeherder.mozilla.org/#/jobs?repo=try&revision=5252f2e9031e
Assignee | ||
Comment 34•9 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #33) > Latest wip, hopefully with the odd plugin related test failures fixed - > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5252f2e9031e Updated push with some changes in nsIPluginTag removed - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c183218ef539 My original changes made it so that we default to blocked for plugins if there's anything wrong with querying the blocklist. Apparently while running tests we run into this, need to dig in a bit to understand why.
Assignee | ||
Comment 35•9 years ago
|
||
Same patch as last time except that I've added an async message sent back from content to chrome when the blocklist updates. The event isn't of much use to anything except tests, which need a way of confirming the blocklist is fully initialized and ready in both processes.
Attachment #8585077 -
Attachment is obsolete: true
Attachment #8585078 -
Attachment is obsolete: true
Attachment #8585079 -
Attachment is obsolete: true
Attachment #8585081 -
Attachment is obsolete: true
Attachment #8587580 -
Flags: review?(dtownsend)
Assignee | ||
Comment 36•9 years ago
|
||
This adds a chrome side blocklist query method to PContent.
Attachment #8587583 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 37•9 years ago
|
||
Updates to the blocklist shim that exists in the content process: 1) generally fail on unexpected calls 2) add a couple component interfaces. Not sure if this is needed but I added it while debugging my try run test failures. the blocklist service supports these, so seems correct that we implement this on the shim as well.
Attachment #8587584 -
Flags: review?(dtownsend)
Assignee | ||
Comment 38•9 years ago
|
||
This is the head.js utility changes. I tried to comment most everything I touched or added. Generally pretty confident I'm leaving this in better shape than it was in.
Attachment #8587591 -
Flags: review?(mconley)
Assignee | ||
Comment 39•9 years ago
|
||
Misc. tests that didn't fall into a test category.
Attachment #8587592 -
Flags: review?(mconley)
Assignee | ||
Updated•9 years ago
|
Attachment #8587592 -
Attachment description: test part 2 → tests part 2
Assignee | ||
Comment 41•9 years ago
|
||
click to play tests.
Attachment #8587594 -
Flags: review?(mconley)
Assignee | ||
Comment 42•9 years ago
|
||
new tests I've added to test blocklisting.
Attachment #8587595 -
Flags: review?(felipc)
Assignee | ||
Comment 43•9 years ago
|
||
Assignee | ||
Comment 44•9 years ago
|
||
Attachment #8587597 -
Attachment is obsolete: true
Assignee | ||
Comment 45•9 years ago
|
||
Almost forgot the most important patch - actually checking the chrome service before instantiating plugins.
Attachment #8587603 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 46•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=17f4ca0ea076 I see green!
Updated•9 years ago
|
Attachment #8587580 -
Flags: review?(dtownsend) → review+
Comment 47•9 years ago
|
||
Comment on attachment 8587584 [details] [diff] [review] shim updates v.1 Review of attachment 8587584 [details] [diff] [review]: ----------------------------------------------------------------- r+ but if we can easily get rid of the nsITimerCallback interface then please do. ::: toolkit/mozapps/extensions/nsBlocklistServiceContent.js @@ +28,5 @@ > classID: Components.ID("{e0a106ed-6ad4-47a4-b6af-2f1c8aa4712d}"), > > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, > + Ci.nsIBlocklistService, > + Ci.nsITimerCallback]), I'd rather not add nsITimerCallback here if we can avoid it. What is calling it like that? If it's the app update service then making this line main process only should solve it: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/extensions.manifest#8. That would also indicate that the update service was running in the content process which might be a bug in itself. @@ +89,4 @@ > }, > > getAddonBlocklistState: function (aAddon, aAppVersion, aToolkitVersion) { > + return Components.interfaces.nsIBlocklistService.STATE_BLOCKED; Ci.nsIBlocklistService.STATE_BLOCKED;
Attachment #8587584 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 48•9 years ago
|
||
Improving these two patches a bit - moving some e10s related logic in with common calls in plugin host.
Attachment #8587583 -
Attachment is obsolete: true
Attachment #8587603 -
Attachment is obsolete: true
Attachment #8587583 -
Flags: review?(wmccloskey)
Attachment #8587603 -
Flags: review?(wmccloskey)
Attachment #8587923 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 49•9 years ago
|
||
a much simpler patch now.
Attachment #8587924 -
Flags: review?(wmccloskey)
Assignee | ||
Updated•9 years ago
|
Attachment #8587924 -
Attachment is patch: true
Assignee | ||
Comment 50•9 years ago
|
||
even simpler after some whitespace cleanup.
Attachment #8587924 -
Attachment is obsolete: true
Attachment #8587924 -
Flags: review?(wmccloskey)
Attachment #8587926 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 51•9 years ago
|
||
sorry for the spam, I just noticed a bug in that last rev.
Attachment #8587923 -
Attachment is obsolete: true
Attachment #8587923 -
Flags: review?(wmccloskey)
Attachment #8587927 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 52•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4535bb99ebb7 This work apparently leaks the world on OSX 10.6, probably due to a test. Other than this, looking good.
Comment 53•9 years ago
|
||
Comment on attachment 8587591 [details] [diff] [review] tests part 1 Review of attachment 8587591 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/plugins/browser.ini @@ +63,5 @@ > +[browser_CTP_crashreporting.js] > +skip-if = e10s || !crashreporter > +[browser_CTP_drag_drop.js] > +skip-if = e10s # misc. issues with tab drag out and e10s > + Nit - why the newlines here, at 76 and 85? ::: browser/base/content/test/plugins/head.js @@ +28,5 @@ > + * @param aSynthEvents listen for synth events too > + * @param aTimeoutMs the number of miliseconds to wait before giving up > + * @returns a Promise that resolves to the received event, or to an Error > + */ > +function promiseForEvent(aSubject, aEventName, aCapture, aSynthEvents, aTimeoutMs, aTarget) { Does this offer anything that https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#l189 doesn't? If so, we might want to augment the code in BrowserTestUtils.jsm instead and get rid of this specialized function so that other tests can take advantage of it. @@ +38,5 @@ > + aSubject.removeEventListener(aEventName, listener, aEventName, aCapture); > + eventDeferred.reject( new Error(aEventName + " event timeout at " + stack) ); > + }, timeoutMs); > + > + var listener = function (aEvent) { let, not var @@ +47,5 @@ > + clearTimeout(timerID); > + eventDeferred.resolve(aEvent); > + } > + > + function cleanup(aEventOrError) { I know we don't have "use strict"; on in this file, but we should probably define this like the listener: let cleanup = function(aEventOrError) { //... }; In the event that we turn strictness on. @@ +75,5 @@ > + > + function done() { > + deferred.resolve(true); > + info("waitForMs finished waiting, waited for " > + + (Date.now() - startTime) Can't we just print out aMs instead of calculating the difference? Or do we need to be really specific with the output here for some reason? @@ +86,5 @@ > +// The blocklist shim running in the content process does not initialize at > +// start up, so it's not active until we load content that needs to do a > +// check. This helper bypasses the delay to get the svc up and running > +// immediately. Note, call this after remote content has loaded. > +function promiseInitContentBlocklistSvc() We might want to pass in a browser here, since (eventually) we'll support multiple separate content processes. @@ +90,5 @@ > +function promiseInitContentBlocklistSvc() > +{ > + return ContentTask.spawn(gTestBrowser, {}, function* () { > + try { > + let bls = Components.classes["@mozilla.org/extensions/blocklist;1"] ContentTask's have access to Cc and Ci, so let's use those shortcuts. @@ +193,3 @@ > function setTestPluginEnabledState(newEnabledState, pluginName) { > + let name = pluginName || "Test Plug-in"; > + var plugin = getTestPlugin(name); let, not var @@ +199,5 @@ > +// Get the 'enabledState' on the nsIPluginTag stored in the main or chrome > +// process. > +function getTestPluginEnabledState(pluginName) { > + let name = pluginName || "Test Plug-in"; > + var plugin = getTestPlugin(name); let, not var @@ +207,5 @@ > +// Returns a promise for the nsIObjectLoadingContent 'pluginFallbackType' > +// property. > +function promiseForObjectFallbackType(aId, aBrowser) { > + let browser = aBrowser || gTestBrowser; > + return ContentTask.spawn(browser, {id: aId}, function* () { Might as well just pass the ID instead of an object with the id as a property. Then, have the function accept those arguments, instead of digging into arguments[0]. @@ +210,5 @@ > + let browser = aBrowser || gTestBrowser; > + return ContentTask.spawn(browser, {id: aId}, function* () { > + let plugin = content.document.getElementById(arguments[0].id); > + if (!(plugin instanceof Ci.nsIObjectLoadingContent)) > + return -1; Should we be throwing/rejecting in this case? @@ +216,5 @@ > + }); > +} > + > +// Returns a promise for the nsIObjectLoadingContent 'activated' property > +function promiseForObjectActivationState(aId, aBrowser) { Instead of individual functions that resolve to these plugin properties, can we have a single function that returns the values for all interesting properties, and then have the caller pick and choose which ones they care about? @@ +255,5 @@ > +// Return a promise and call the plugin's nsIObjectLoadingContent > +// playPlugin() method. > +function promisePlayObject(aId, aBrowser) { > + let browser = aBrowser || gTestBrowser; > + return ContentTask.spawn(browser, {id: aId}, function* () { As mentioned elsewhere, just pass the ID as the second argument to spawn, and accept it as the first (and only parameter) in the function - no need to dig about in arguments[0]. @@ +267,5 @@ > +function promiseObjectValueResult(aId, aBrowser) { > + let browser = aBrowser || gTestBrowser; > + return ContentTask.spawn(browser, {id: aId}, function* () { > + let plugin = content.document.getElementById(arguments[0].id); > + dump("---"+Components.utils.waiveXrays(plugin).getObjectValue()); Nit - remove dump, unless we really need this. @@ +275,5 @@ > + > +// Return a promise and reload the target plugin in the page > +function promiseReloadPlugin(aId, aBrowser) { > + let browser = aBrowser || gTestBrowser; > + return ContentTask.spawn(browser, {id: aId}, function* () { Same as above, re: passing ID as the second argument. @@ +288,5 @@ > let perms = Services.perms.enumerator; > while (perms.hasMoreElements()) { > let perm = perms.getNext(); > if (perm.type.startsWith('plugin')) { > + dump("perm " + perm.host + " " + perm.type + "\n"); Is this useful? If not, remove it. If so, maybe use info() instead. @@ +343,5 @@ > function resetBlocklist() { > Services.prefs.setCharPref("extensions.blocklist.url", _originalTestBlocklistURL); > } > > +// Insure there's a popup notification present and that it's displayed Nit - trailing ws @@ +344,5 @@ > Services.prefs.setCharPref("extensions.blocklist.url", _originalTestBlocklistURL); > } > > +// Insure there's a popup notification present and that it's displayed > +function promisePopupNotificationShown(aName, aAction) { What's the point of action? Why can't the caller just do action after the promise resolves? And shouldn't we make it possible to pass in the browser here in case we're dealing with multiple browser tabs? Then pass the browser as the second argument to getNotification. @@ +403,5 @@ > + * @param aObsEvent the observer event to wait for > + * @param aTimeoutMs the number of miliseconds to wait before giving up > + * @returns a Promise that resolves to true, or to an Error > + */ > +function promiseForObserver(aObsEvent, aTimeoutMs, aObsData) { We have https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/testing/modules/TestUtils.jsm#l48 to do this. However, that one doesn't have a timeout, which I think is a good idea. We might want to augment topicObserved with timeout capabilities instead of adding a new function that does essentially the same thing. @@ +512,5 @@ > notification.reshow(); > } > > +function promiseForNotificationShown(notification) > +{ Nit - brace on line 515 @@ +519,5 @@ > + return deferred.promise; > +} > + > +function promiseToReshowNotfication(aNotification) > +{ Nit - brace on line 522 @@ +525,5 @@ > + let condition = function() { > + return !aNotification.dismissed && PopupNotifications.panel.firstChild; > + } > + aNotification.reshow(); > + return promiseForCondition(condition); Shouldn't this be promisePopupNotificationShown? @@ +562,5 @@ > * The URI to load. > * > * @return Promise > */ > +function promiseLoadPage(browser, uri) { Isn't this a subset of what promiseTabLoadEvent does? Maybe we should just add a method to do load a URI and wait for a particular event to BrowserTestUtils instead of doing it here. BrowserTestUtils is where all of the super-common browser testing code should go, as a way to stop reinventing the wheel. :)
Attachment #8587591 -
Flags: review?(mconley) → review-
Comment 54•9 years ago
|
||
Comment on attachment 8587592 [details] [diff] [review] tests part 2 Review of attachment 8587592 [details] [diff] [review]: ----------------------------------------------------------------- Hey jimm! Thanks so much for cleaning this stuff up. These are ending up wayyyyyyyyyy better than their current state, so big thumbs up. :) I was a bit repetitive during parts of my review, since the same issues kept coming up. I don't blame you though - the tests you were working off of were pretty old and crufty. This couldn't have been a whole lot of fun. ::: browser/base/content/test/plugins/browser_clearplugindata.js @@ +1,2 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this Test files are all supposed to be public domain, I believe. @@ +4,2 @@ > > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); I know we're in the global scope here, but let is (in my books) greatly preferred over var. Please try not to introduce new vars. @@ +4,3 @@ > > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); You should have access to Cc and Ci here. @@ +4,4 @@ > > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > +var gTestBrowser = null; I can't see much purpose to gTestBrowser in our add_task world. @@ +19,5 @@ > var pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost); > pluginHost.QueryInterface(pluginHostIface); > > var pluginTag = getTestPlugin(); > +var sanitizer = null; Hrm. Now we have sanitizer and Sanitizer. I think we should maybe name these two different things... Maybe this is gSanitizer? @@ +36,5 @@ > } > return true; > } > > +Components.utils.import("resource://gre/modules/Services.jsm"); Services should already be available to the test. @@ +41,3 @@ > > +add_task(function* () { > + registerCleanupFunction(Task.async(function*() { Why is this an async generator function? None of the method calls in this function, as far as I can tell, are async or yield. @@ +62,5 @@ > + > + sanitizer = new Sanitizer(); > + sanitizer.ignoreTimespan = false; > + sanitizer.prefDomain = "privacy.cpd."; > + let itemPrefs = gPrefService.getBranch(sanitizer.prefDomain); Services.prefs @@ +89,4 @@ > > + // Clear 20 seconds ago > + let now_uSec = Date.now() * 1000; > + sanitizer.range = [now_uSec - 20*1000000, now_uSec]; Spaces on either side of * @@ +104,2 @@ > > + gBrowser.removeCurrentTab(); Instead of adding the tab and removing it at the end of the test, use BrowserTestUtils.withNewTab. Example: https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/toolkit/components/passwordmgr/test/browser/browser_notifications.js#l147 This will take care of opening the tab, waiting for it to load, and then closing it for you when it's done. @@ +113,2 @@ > > + yield promiseTabLoadEvent(gBrowser.selectedTab, testURL2); Same as above, re: BrowserTestUtils.withNewTab. ::: browser/base/content/test/plugins/browser_plugin_infolink.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Public domain license. @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); gPluginHost is never used in this file, and Cc / Ci are available to you. @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > +var gTestBrowser = null; No need for gTestBrowser. @@ +5,5 @@ > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > +var gTestBrowser = null; > + > +Components.utils.import("resource://gre/modules/Services.jsm"); No need to import Services.jsm. @@ +8,5 @@ > + > +Components.utils.import("resource://gre/modules/Services.jsm"); > + > +add_task(function* () { > + registerCleanupFunction(Task.async(function*() { No need for this to be async, nor for it to reference gTestBrowser or to remove the current tab. @@ +19,5 @@ > + })); > +}); > + > +add_task(function* () { > + gBrowser.selectedTab = gBrowser.addTab(); Let's use BrowserTestUtils.withNewTab. @@ +35,5 @@ > + > + yield asyncSetAndUpdateBlocklist(gTestRoot + "blockNoPlugins.xml", gTestBrowser); > +}); > + > +add_task(function* () { Can you quickly add some documentation to describe what this test tests? @@ +36,5 @@ > + yield asyncSetAndUpdateBlocklist(gTestRoot + "blockNoPlugins.xml", gTestBrowser); > +}); > + > +add_task(function* () { > + yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_test.html"); BrowserTestUtils.withNewTab @@ +42,5 @@ > + yield promiseUpdatePluginBindings(gTestBrowser); > + > + let fallbackType = yield promiseForObjectFallbackType("test"); > + is(fallbackType, Ci.nsIObjectLoadingContent.PLUGIN_DISABLED, > + "Test 1a, plugin fallback type should be PLUGIN_DISABLED"); Remove "Test 1a" @@ +64,5 @@ > + info("TabOpen received."); > + > + promise = promiseForEvent(gBrowser.tabContainer, "TabClose", true); > + > + // in-process page, no cpows here For now, anyway. Not sure if we ever plan to make about:addons remote, but it wouldn't surprise me. @@ +68,5 @@ > + // in-process page, no cpows here > + let win = gBrowser.selectedBrowser.contentWindow; > + let condition = function() { > + win = gBrowser.selectedBrowser.contentWindow; > + if (!!win && !!win.wrappedJSObject && !!win.wrappedJSObject.gViewController) { You can probably just: if (win && win.wrappedJSObject && win.wrappedJSObject.gViewController) { // ... } ::: browser/base/content/test/plugins/browser_plugin_reloading.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Public domain. @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); gPluginHost is never used in this file. And Cc and Ci are available to you. @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > +var gTestBrowser = null; No gTestBrowser please. @@ +5,5 @@ > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > +var gTestBrowser = null; > + > +Components.utils.import("resource://gre/modules/Services.jsm"); No need to import this. @@ +13,5 @@ > + setTestPluginEnabledState(aState, "Second Test Plug-in"); > +} > + > +add_task(function* () { > + registerCleanupFunction(Task.async(function*() { No need to be async or generator. Or to remove the tab. Or to reset gTestBrowser. @@ +27,5 @@ > + })); > +}); > + > +add_task(function* () { > + gBrowser.selectedTab = gBrowser.addTab(); Use BrowserTestUtils.withNewTab. @@ +49,5 @@ > + clearAllPluginPermissions(); > + > + updateAllTestPlugins(Ci.nsIPluginTag.STATE_CLICKTOPLAY); > + > + yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_test.html"); Use BrowserTestUtils.withNewTab @@ +70,5 @@ > + let displayType = yield promiseForObjectDisplayType("test"); > + is(displayType, Ci.nsIObjectLoadingContent.TYPE_PLUGIN, > + "Test 3, plugin should have started"); > + let isActivated = yield promiseForObjectActivationState("test"); > + ok(isActivated, "Test 4, plugin node should not be activated"); Please remove "Test 1", "Test 2", etc from your checks / messages. @@ +74,5 @@ > + ok(isActivated, "Test 4, plugin node should not be activated"); > + > + let result = yield ContentTask.spawn(gTestBrowser, {}, function* () { > + let plugin = content.document.getElementById("test"); > + let npobj1 = Components.utils.waiveXrays(plugin).getObjectValue(); Cu is available to you in this scope. ::: browser/base/content/test/plugins/browser_pluginnotification.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Public domain. @@ +4,3 @@ > > +var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); This is never used. @@ +7,1 @@ > var gTestBrowser = null; No gTestBrowser please. @@ +7,4 @@ > var gTestBrowser = null; > + > +// the main test content tab. > +let gTestTab = null; Let's not hold a global on tabs either. @@ +11,2 @@ > > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import this. @@ +11,4 @@ > > Components.utils.import("resource://gre/modules/Services.jsm"); > > +function updateAllTestPlugins(aState) { Maybe this is something that could go into head.js? I've seen you use it a few times. @@ +17,4 @@ > } > > +add_task(function* () { > + registerCleanupFunction(Task.async(function*() { No need for async or generator. Also, no need to reference gTestBrowser, gTestTab or to remove the tab if you use BrowserTestUtils.withNewTab. @@ +33,3 @@ > > +add_task(function* () { > + let newTab = gBrowser.addTab(); Use BrowserTestUtils.withNewTab. @@ +52,4 @@ > > // Tests a page with an unknown plugin in it. > +add_task(function* () { > + yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_unknown.html"); Use BrowserTestUtils.withNewTab @@ +59,4 @@ > > + let fallbackType = yield promiseForObjectFallbackType("unknown"); > + is(fallbackType, Ci.nsIObjectLoadingContent.PLUGIN_UNSUPPORTED, > + "Test 1a, plugin fallback type should be PLUGIN_UNSUPPORTED"); Remove "Test #" prefixes from messages. @@ +87,5 @@ > // Tests that the "Allow Always" permission works for click-to-play plugins > +add_task(function* () { > + updateAllTestPlugins(Ci.nsIPluginTag.STATE_CLICKTOPLAY); > + > + yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_test.html"); Use BrowserTestUtils.withNewTab @@ +111,5 @@ > > // Test that the "Always" permission, when set for just the Test plugin, > // does not also allow the Second Test plugin. > +add_task(function* () { > + yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_two_types.html"); Use BrowserTestUtils.withNewTab. I won't mention this again for the rest of this file. @@ +195,5 @@ > + let left = (bounds.left + bounds.right) / 2; > + let top = (bounds.top + bounds.bottom) / 2; > + let utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor) > + .getInterface(Components.interfaces.nsIDOMWindowUtils); > + utils.sendMouseEvent("mousedown", left, top, 0, 1, 0, false, 0, 0); I wonder if we should just add a ContentTaskUtils.clickAtCenter(element); to ContentTaskUtils.jsm? ::: browser/base/content/test/plugins/browser_plugins_added_dynamically.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public Tests go into the public domain. @@ +8,2 @@ > > let gTestBrowser = null; We likely don't need gTestBrowser. Just use BrowserTestUtils.withNewTab (whose function gets passed the browser), or gBrowser.selectedBrowser if you just need the current browser. @@ +10,2 @@ > > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import this - Services is available already. @@ +10,5 @@ > > Components.utils.import("resource://gre/modules/Services.jsm"); > > +add_task(function* () { > + registerCleanupFunction(Task.async(function*() { Why is this async? And a generator? @@ +30,1 @@ > let newTab = gBrowser.addTab(); Why are we adding a tab here? It's not being used until the next test. If we need to do this before we setTestPluginEnabledState, can't the calls in the next add_task just be added into this test? Otherwise, let's just open the new tab in the next addTask (and probably use BrowserTestUtils.withNewTab while we're at it). @@ +43,3 @@ > > + let notification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser); > + ok(!notification, "Test 1a, Should not have a click-to-play notification"); "Test 1a" doesn't really have much meaning here... @@ +46,4 @@ > > + // Add a plugin of type test > + yield ContentTask.spawn(gTestBrowser, {}, function* () { > + new XPCNativeWrapper(XPCNativeWrapper.unwrap(content).addPlugin("pluginone", "application/x-test")); I'm curious - why do we have to wrap this in the construction of a new XPCNativeWrapper? I don't see the old addPlugin doing this... @@ +53,3 @@ > > + notification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser); > + ok(notification, "Test 1a, Should not have a click-to-play notification"); Remove "Test 1a" @@ +62,5 @@ > centerAction = action; > break; > } > } > + ok(centerAction, "Test 2, found center action for the Test plugin"); Remove "Test 2" @@ +67,4 @@ > > let centerItem = null; > for (let item of PopupNotifications.panel.firstChild.childNodes) { > + is(item.value, "block", "Test 3, all plugins should start out blocked"); Remove "Test 3" @@ +72,5 @@ > centerItem = item; > break; > } > } > + ok(centerItem, "Test 4, found center item for the Test plugin"); Remove "Test 4" - basically, just remove the old test subfunction name things. @@ +91,2 @@ > > + isActivated |= yield promiseForObjectActivationState("plugintwo"); Why |=? Why not just =? We already know whether or not pluginone is activated from line 85. @@ +100,2 @@ > > + yield promiseTabLoadEvent(gBrowser.selectedTab, gTestRoot + "plugin_add_dynamically.html"); BrowserTestUtils.withNewTab. @@ +119,5 @@ > + > + let isActivated = yield promiseForObjectActivationState("pluginone"); > + ok(!isActivated, "Test 8, test plugin should be activated"); > + > + let condition = function() !notification.dismissed && Why are we checking this? What did we expect to have changed this?
Attachment #8587592 -
Flags: review?(mconley) → review-
Assignee | ||
Comment 55•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #53) > Comment on attachment 8587591 [details] [diff] [review] > tests part 1 > > Review of attachment 8587591 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/test/plugins/browser.ini > @@ +63,5 @@ > > +[browser_CTP_crashreporting.js] > > +skip-if = e10s || !crashreporter > > +[browser_CTP_drag_drop.js] > > +skip-if = e10s # misc. issues with tab drag out and e10s > > + > > Nit - why the newlines here, at 76 and 85? Just breaking up the different blocks of test for organizational purposes. > ::: browser/base/content/test/plugins/head.js > @@ +28,5 @@ > > + * @param aSynthEvents listen for synth events too > > + * @param aTimeoutMs the number of miliseconds to wait before giving up > > + * @returns a Promise that resolves to the received event, or to an Error > > + */ > > +function promiseForEvent(aSubject, aEventName, aCapture, aSynthEvents, aTimeoutMs, aTarget) { > > Does this offer anything that > https://hg.mozilla.org/mozilla-central/file/4fe763cbe196/testing/mochitest/ > BrowserTestUtils/BrowserTestUtils.jsm#l189 doesn't? If so, we might want to > augment the code in BrowserTestUtils.jsm instead and get rid of this > specialized function so that other tests can take advantage of it. I wasn't aware of this file but there's not much in here so no major harm. I'll see if I can reuse what we have here and repost.
Comment on attachment 8587580 [details] [diff] [review] fix for blocklist updates v.2 Review of attachment 8587580 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/nsBlocklistServiceContent.js @@ +34,5 @@ > + Services.obs.addObserver(this, "xpcom-shutdown", false); > + }, > + > + uninit: function () { > + Services.cpmm.removeMessageListener("Blocklist::blocklistInvalidated", this); Just wanted to point out that the usual convention for message names is Foo:bar (only one colon).
Comment on attachment 8587926 [details] [diff] [review] query the chrome blocklist service v.2 Review of attachment 8587926 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/base/nsPluginHost.cpp @@ +1282,5 @@ > if (pluginTag) { > + // When setting up a bridge, double check with chrome to see if this plugin > + // is blocked hard. Note this does not protect against vulnerable plugins > + // that the user has explicitly allowed. :( > + if (pluginTag->IsBlocklistedInChrome()) { Since you're already in the parent process, couldn't you just use IsBlocklisted() here?
Attachment #8587926 -
Flags: review?(wmccloskey) → review+
Comment on attachment 8587927 [details] [diff] [review] implement remote blocklist query v.2 Review of attachment 8587927 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +1017,5 @@ > return mozilla::plugins::SetupBridge(aPluginId, this, true, aRv); > } > > bool > +ContentParent::RecvGetBlocklistState(const nsCString& aPluginName, Each nsPluginTag has an mId field that is shared between content and chrome. It would be better to use that instead of the name. Then you can use nsPluginHost::PluginWithId here and you don't need to add FindTagWithName. ::: dom/plugins/base/nsPluginHost.h @@ +28,5 @@ > #include "nsIIDNService.h" > #include "nsCRT.h" > > #ifdef XP_WIN > +#include <minwindef.h> What's this for? ::: dom/plugins/base/nsPluginTags.cpp @@ +469,5 @@ > + return bls == nsIBlocklistService::STATE_BLOCKED; > +} > + > +nsresult > +nsPluginTag::GetBlocklistedStateInChrome(uint32_t *aResult) If it's possible, it seems cleaner to me to change nsPluginTag::GetBlocklistedState to have a check at the top to see if we're in the content process. If we are, it could do the IPC thing and return. Then we wouldn't need the special *InChrome functions. I think the patch would be smaller, since you wouldn't have to change any callers. The way you have it now, it's more likely that people will call GetBlocklistedState in the content process and get the wrong answer. For example, it looks like nsPluginHost::GetPermissionStringForType does the wrong thing.
Attachment #8587927 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 59•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #58) > Comment on attachment 8587927 [details] [diff] [review] > implement remote blocklist query v.2 > > Review of attachment 8587927 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/ContentParent.cpp > @@ +1017,5 @@ > > return mozilla::plugins::SetupBridge(aPluginId, this, true, aRv); > > } > > > > bool > > +ContentParent::RecvGetBlocklistState(const nsCString& aPluginName, > > Each nsPluginTag has an mId field that is shared between content and chrome. > It would be better to use that instead of the name. Then you can use > nsPluginHost::PluginWithId here and you don't need to add FindTagWithName. Ah, nice. Thanks. > ::: dom/plugins/base/nsPluginHost.h > @@ +28,5 @@ > > #include "nsIIDNService.h" > > #include "nsCRT.h" > > > > #ifdef XP_WIN > > +#include <minwindef.h> > > What's this for? I had to remove napi.h from nsPluginHost.h so that I was able include it in ContentParent.cpp. The napi header pulls in some osx sdk junk specific to a certain sdk version. This broke the build. Removing that include solved the problem. > ::: dom/plugins/base/nsPluginTags.cpp > @@ +469,5 @@ > > + return bls == nsIBlocklistService::STATE_BLOCKED; > > +} > > + > > +nsresult > > +nsPluginTag::GetBlocklistedStateInChrome(uint32_t *aResult) > > If it's possible, it seems cleaner to me to change > nsPluginTag::GetBlocklistedState to have a check at the top to see if we're > in the content process. If we are, it could do the IPC thing and return. > Then we wouldn't need the special *InChrome functions. I think the patch > would be smaller, since you wouldn't have to change any callers. > > The way you have it now, it's more likely that people will call > GetBlocklistedState in the content process and get the wrong answer. For > example, it looks like nsPluginHost::GetPermissionStringForType does the > wrong thing. This will trigger a lot more traffic over this call.. I'm not sure what effect that'll have. I'll push it at try though, see what happens.
Assignee | ||
Comment 60•9 years ago
|
||
Comment on attachment 8587593 [details] [diff] [review] tests part 3 since head.js is changes, these files will change as well.
Attachment #8587593 -
Flags: review?(felipc)
Assignee | ||
Updated•9 years ago
|
Attachment #8587594 -
Flags: review?(mconley)
Assignee | ||
Updated•9 years ago
|
Attachment #8587595 -
Flags: review?(felipc)
Assignee | ||
Comment 61•9 years ago
|
||
I'm kind of surprised these changes didn't trigger bug 1150861: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c316875c5364 even with this patch, which changed the default in a number of locations: https://hg.mozilla.org/try/rev/414dfb3b448a Which is good news since we can move forward.
Assignee | ||
Comment 62•9 years ago
|
||
Attachment #8587591 -
Attachment is obsolete: true
Attachment #8587592 -
Attachment is obsolete: true
Attachment #8587593 -
Attachment is obsolete: true
Attachment #8587594 -
Attachment is obsolete: true
Attachment #8587595 -
Attachment is obsolete: true
Attachment #8587601 -
Attachment is obsolete: true
Attachment #8587927 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8595422 -
Attachment is obsolete: true
Assignee | ||
Comment 63•9 years ago
|
||
/r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop /r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service. /r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop /r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm /r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley Pull down these commits: hg pull -r 960bd216241dbdfe69f2be3b24d42b005bb7561e https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 64•9 years ago
|
||
Comment on attachment 8595517 [details] MozReview Request: bz://1129040/jimm /r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop /r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service. /r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop /r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm /r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley Pull down these commits: hg pull -r 960bd216241dbdfe69f2be3b24d42b005bb7561e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8595517 -
Flags: review?(wmccloskey)
Attachment #8595517 -
Flags: review?(mconley)
Assignee | ||
Comment 65•9 years ago
|
||
Comment on attachment 8595517 [details] MozReview Request: bz://1129040/jimm /r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop /r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service. /r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop /r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm /r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley Pull down these commits: hg pull -r 960bd216241dbdfe69f2be3b24d42b005bb7561e https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/7393/#review6161 ::: dom/ipc/ContentParent.cpp:1074 (Diff revision 1) > + Extra whitespace. ::: dom/ipc/ContentParent.cpp:1075 (Diff revision 1) > + return NS_SUCCEEDED(tag->GetBlocklistState(aState)) ? true : false; No need for the ? true : false stuff. ::: dom/plugins/base/nsPluginHost.h:225 (Diff revision 1) > + nsPluginTag* FindTagWithName(const nsCString& aName); This isn't used anymore. ::: dom/ipc/ContentParent.cpp:159 (Diff revision 1) > +#include "nsPluginHost.h" Now I remember the problem you mentioned with including plugin stuff in ContentParent.cpp. I fixed it by creating a new file, PluginBridge.h, that exports stuff that ContentParent.cpp can use. That's not really any better than what you've done in nsPluginHost.h, but it's an option if you prefer it. ::: dom/plugins/base/nsPluginHost.cpp:1722 (Diff revision 1) > +nsPluginHost::FindTagWithName(const nsCString& aName) Also unused. ::: dom/plugins/base/nsPluginTags.cpp:655 (Diff revision 1) > + if (cp) { I don't think there's any need to null-check |cp|. It's always there. ::: dom/plugins/base/nsPluginTags.cpp:650 (Diff revision 1) > + // can fail quite easily. Uhhh, what?
Comment on attachment 8595517 [details]
MozReview Request: bz://1129040/jimm
...yet another confusing ReviewBoard experience. Isn't it supposed to set the r+ flag somehow?
Attachment #8595517 -
Flags: review?(wmccloskey) → review+
Comment 69•9 years ago
|
||
https://reviewboard.mozilla.org/r/7399/#review6267 ::: browser/base/content/test/plugins/blocklist_proxy.js:5 (Diff revision 1) > +let Cc = Components.classes; > +let Ci = Components.interfaces; > +let Cu = Components.utils; > +let Cm = Components.manager; These should probably be consts. ::: browser/base/content/test/plugins/blocklist_proxy.js:20 (Diff revision 1) > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, You never import XPCOMUtils in this file - you should probably do that just in case. ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:1 (Diff revision 1) > -var rootDir = getRootDirectory(gTestPath); > -const gTestRoot = rootDir; > -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Nit - I don't think tests are supposed to be MPL'd - I think they're supposed to be public domain. I'm pretty sure you can just strip the license from any of these tests. ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:12 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { The generator that you're passing to Task.async doesn't yield. Shouldn't this just be a normal function? ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:63 (Diff revision 1) > - > + yield promiseForCondition(() => window.document.getElementById("context-ctp-play")); No need for "window." prefix here or on line 65. ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:66 (Diff revision 1) > - ok(activate, "Test 2, Should have a context menu entry for activating the plugin"); > + ok(actMenuItem, "Should have a context menu entry for activating the plugin"); Isn't this implied by line 63? ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:7 (Diff revision 1) > -var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > +let gTestBrowser = null; Let's get rid of gTestBrowser - there's only one function now. ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:20 (Diff revision 1) > + gTestBrowser = null; Let's get rid of gTestBrowser. ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:30 (Diff revision 1) > gTestBrowser = gBrowser.selectedBrowser; This can probably just be "browser" - no need to share gTestBrowser between functions now. ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:1 (Diff revision 1) > -var rootDir = getRootDirectory(gTestPath); > -const gTestRoot = rootDir; > -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Remove license. ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:10 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); Services is just available to all tests - I think this can be removed. ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:9 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); Services should be available to all tests - this can just be removed, I think. ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:13 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { Just use a normal function instead of a generator passed to Task.async. ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:29 (Diff revision 1) > Services.prefs.setBoolPref("plugins.click_to_play", true); > - setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY); > + Services.prefs.setBoolPref("extensions.blocklist.suppressUI", true); > - setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Second Test Plug-in"); > > - prepareTest(test1a, gHttpTestRoot + "plugin_data_url.html"); > -} > + setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Test Plug-in"); > + setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Second Test Plug-in"); This setup stuff can probably just be added to the same add_task that adds your cleanup function. Might as well keep setup and teardown together. ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:67 (Diff revision 1) > + let plugin = content.document.getElementById("test"); > + let bounds = plugin.getBoundingClientRect(); > + let left = (bounds.left + bounds.right) / 2; > + let top = (bounds.top + bounds.bottom) / 2; > + let utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor) > + .getInterface(Components.interfaces.nsIDOMWindowUtils); > + utils.sendMouseEvent("mousedown", left, top, 0, 1, 0, false, 0, 0); > + utils.sendMouseEvent("mouseup", left, top, 0, 1, 0, false, 0, 0); A note for future reference that Enn landed some tools to do clicking on the content process in bug 1131818. I don't think you should worry about using those here though - just for the future. ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:79 (Diff revision 1) > - waitForNotificationShown(popupNotification, function() { > + let condition = function() !PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser).dismissed && > + PopupNotifications.panel.firstChild; I think we can effectively abandon putting this on one line, and just use: ``` let condition = () => { let notification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser); return !notification.dismissed && PopupNotifications.panel.firstChild; } ``` ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:164 (Diff revision 1) > function test2c() { > - let plugin = gTestBrowser.contentDocument.getElementById("test1"); > + // We click activated above This test looks like it's never run now... ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:175 (Diff revision 1) > function test3a() { This test looks like it's never run now... ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:217 (Diff revision 1) > -function test3c() { > +function test3b() { This test looks like it's never run now...
Comment 70•9 years ago
|
||
https://reviewboard.mozilla.org/r/7399/#review6275 ::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:8 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import Services. ::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:1 (Diff revision 1) > -/* Any copyright is dedicated to the Public Domain. > - * http://creativecommons.org/publicdomain/zero/1.0/ */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ No MPL - the public domain license was correct. ::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:11 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { No need for a generator / Task.async. Just use a normal function here. ::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:42 (Diff revision 1) > - waitForCondition(condition, part3, "Waited too long for click-to-play notification"); > -} > + // XXX technically can't load fire before we get this call??? > + yield waitForEvent(gNewWindow, "load", null, true); Maybe. You could work around that possibility by setting the promise before you call replaceTabWithWindow, and then yielding it. ::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:1 (Diff revision 1) > -var rootDir = getRootDirectory(gTestPath); > -const gTestRoot = rootDir; > -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ No MPL. ::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:8 (Diff revision 1) > +let gTestBrowser = null; There's just a single add_task - no need for a global reference to the browser. ::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:10 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import Services. ::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:13 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { Just use a normal function here. ::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:19 (Diff revision 1) > + gBrowser.removeCurrentTab(); Instead of removing the current tab in the teardown, since this is a short test, just use BrowserTestUtils.withNewTab. ::: browser/base/content/test/plugins/browser_CTP_hide_overlay.js:30 (Diff revision 1) > gTestBrowser = gBrowser.selectedBrowser; Just use a local variable instead of the gTestBrowser global. ::: browser/base/content/test/plugins/browser_CTP_iframe.js:1 (Diff revision 1) > -let rootDir = getRootDirectory(gTestPath); > -const gTestRoot = rootDir; > -const gHttpTestRoot = rootDir.replace("chrome://mochitests/content/", "http://127.0.0.1:8888/"); > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ No MPL. ::: browser/base/content/test/plugins/browser_CTP_iframe.js:9 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import Services. ::: browser/base/content/test/plugins/browser_CTP_iframe.js:12 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { Just use a normal function here. No need to pass a generator or use Task.async. ::: browser/base/content/test/plugins/browser_CTP_iframe.js:7 (Diff revision 1) > let gTestBrowser = null; No need for gTestBrowser global. ::: browser/base/content/test/plugins/browser_CTP_iframe.js:29 (Diff revision 1) > gTestBrowser = gBrowser.selectedBrowser; Just use a browser local variable, or even better, use BrowserTestUtils.withNewTab. ::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:4 (Diff revision 1) > - > +let gTestBrowser = null; No need for gTestBrowser. ::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:6 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import Services. ::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:9 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { We can just pass a normal function to registerCleanupFunction. ::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:26 (Diff revision 1) > gTestBrowser = gBrowser.selectedBrowser; No need to set a gTestBrowser global - better yet, use BrowserTestUtils.withNewTab. ::: browser/base/content/test/plugins/browser_CTP_multi_allow.js:63 (Diff revision 1) > - > + Trailing whitespace. ::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:4 (Diff revision 1) > - > +let gTestBrowser = null; No need for gTestBrowser global. ::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:6 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import Services. ::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:9 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { Just pass a normal function to registerCleanupFunction. ::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:27 (Diff revision 1) > gTestBrowser = gBrowser.selectedBrowser; No need to use gTestBrowser global - better yet, use BrowserTestUtils.withNewTab. ::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:5 (Diff revision 1) > Components.utils.import("resource://gre/modules/Services.jsm"); No need to import Services. ::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:8 (Diff revision 1) > - waitForExplicitFinish(); > + registerCleanupFunction(Task.async(function*() { Just pass a normal function to registerCleanupFunction. ::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:40 (Diff revision 1) > + Trailing whitespace. ::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:52 (Diff revision 1) > + Trailing whitespace. ::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:78 (Diff revision 1) > "Test 3b, plugin fallback type should be PLUGIN_CLICK_TO_PLAY"); Nit - these "Test X" prefixes no longer make sense in our add_task-y world.
Assignee | ||
Comment 71•9 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #70) > ::: browser/base/content/test/plugins/browser_CTP_drag_drop.js:42 > (Diff revision 1) > > - waitForCondition(condition, part3, "Waited too long for click-to-play notification"); > > -} > > + // XXX technically can't load fire before we get this call??? > > + yield waitForEvent(gNewWindow, "load", null, true); > > Maybe. You could work around that possibility by setting the promise before > you call replaceTabWithWindow, and then yielding it. There's no window to set it on, the call to replaceTabWithWindow creates it.
Assignee | ||
Comment 72•9 years ago
|
||
Updated except the following (In reply to Mike Conley (:mconley) - Needinfo me! from comment #69) > ::: browser/base/content/test/plugins/browser_CTP_context_menu.js:7 > (Diff revision 1) > > -var gPluginHost = Components.classes["@mozilla.org/plugin/host;1"].getService(Components.interfaces.nsIPluginHost); > > +let gTestBrowser = null; > > Let's get rid of gTestBrowser - there's only one function now. I've updated the test you've mentioned here to remove gTestBrowser. However be careful if you work on these, head.js relies on gTestBrowser in a few helpers if you don't pass a browser to them so removing more of these will likely have to include updating the head.js callers that are currently relying on this hidden behavior. > ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:67 > (Diff revision 1) > > + let plugin = content.document.getElementById("test"); > > + let bounds = plugin.getBoundingClientRect(); > > + let left = (bounds.left + bounds.right) / 2; > > + let top = (bounds.top + bounds.bottom) / 2; > > + let utils = content.QueryInterface(Components.interfaces.nsIInterfaceRequestor) > > + .getInterface(Components.interfaces.nsIDOMWindowUtils); > > + utils.sendMouseEvent("mousedown", left, top, 0, 1, 0, false, 0, 0); > > + utils.sendMouseEvent("mouseup", left, top, 0, 1, 0, false, 0, 0); > > A note for future reference that Enn landed some tools to do clicking on the > content process in bug 1131818. I don't think you should worry about using > those here though - just for the future. - skipped > ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:79 > (Diff revision 1) > > - waitForNotificationShown(popupNotification, function() { > > + let condition = function() !PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser).dismissed && > > + PopupNotifications.panel.firstChild; > > I think we can effectively abandon putting this on one line, and just use: > > ``` > let condition = () => { > let notification = > PopupNotifications.getNotification("click-to-play-plugins", > gTestBrowser); > return !notification.dismissed && PopupNotifications.panel.firstChild; > } > ``` - skipped (kinda bike sheddy imo) (In reply to Mike Conley (:mconley) - Needinfo me! from comment #70) > ::: browser/base/content/test/plugins/browser_CTP_nonplugins.js:27 > (Diff revision 1) > > gTestBrowser = gBrowser.selectedBrowser; > > No need to use gTestBrowser global - better yet, use > BrowserTestUtils.withNewTab. - skipped > ::: browser/base/content/test/plugins/browser_CTP_notificationBar.js:78 > (Diff revision 1) > > "Test 3b, plugin fallback type should be PLUGIN_CLICK_TO_PLAY"); > > Nit - these "Test X" prefixes no longer make sense in our add_task-y world. - skipped
Assignee | ||
Comment 73•9 years ago
|
||
Comment on attachment 8595517 [details] MozReview Request: bz://1129040/jimm /r/7391 - Bug 1129040 - Forward blocklist update notifications to the content process. r=Mossop /r/7393 - Bug 1129040 - Provide a way for content processes to query the chrome side blocklist service. /r/7395 - Bug 1129040 - In the content process blocklist shim fail in all addon related calls. r=Mossop /r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to instantiating plugins in the content process. r=billm /r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable for e10s. r=mconley Pull down these commits: hg pull -r 222b533e2239c4f972ab18ff5673adad3580e5a3 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8595517 -
Flags: review+ → review?(wmccloskey)
Assignee | ||
Updated•9 years ago
|
Attachment #8595517 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 74•9 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #73) > Comment on attachment 8595517 [details] > MozReview Request: bz://1129040/jimm > > /r/7391 - Bug 1129040 - Forward blocklist update notifications to the > content process. r=Mossop > /r/7393 - Bug 1129040 - Provide a way for content processes to query the > chrome side blocklist service. > /r/7395 - Bug 1129040 - In the content process blocklist shim fail in all > addon related calls. r=Mossop > /r/7397 - Bug 1129040 - Query the chrome process blocklist service prior to > instantiating plugins in the content process. r=billm > /r/7399 - Bug 1129040 - Update browser/base/content/test/plugins and enable > for e10s. r=mconley > > Pull down these commits: > > hg pull -r 222b533e2239c4f972ab18ff5673adad3580e5a3 > https://reviewboard-hg.mozilla.org/gecko/ I'm concerned that this didn't create an interdiff. Please let me know if it didn't and we can try and figure out why.
Assignee | ||
Comment 75•9 years ago
|
||
Comment 76•9 years ago
|
||
Comment on attachment 8595517 [details] MozReview Request: bz://1129040/jimm https://reviewboard.mozilla.org/r/7389/#review6407 Thanks jimm - and sorry for my pedantry. I think you've made these tests strictly better. Let's drop the hammer. :) ::: browser/base/content/test/plugins/browser_CTP_data_urls.js:216 (Diff revisions 1 - 2) > +// Fails, bug XXX. Plugins plus a data url don't fire a load event. We should get a bug filed on this, and just remove the dead code.
Attachment #8595517 -
Flags: review?(mconley) → review+
Comment 77•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a9dd81198a https://hg.mozilla.org/integration/mozilla-inbound/rev/61b5398161f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a8aa5f3c1c14 https://hg.mozilla.org/integration/mozilla-inbound/rev/a7aab0099e9e https://hg.mozilla.org/integration/mozilla-inbound/rev/5000b4926d38
Comment 78•9 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9401441&repo=mozilla-inbound
Flags: needinfo?(jmathies)
Comment 79•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/64091b0073b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee2862b568c https://hg.mozilla.org/integration/mozilla-inbound/rev/93461cd7a58f https://hg.mozilla.org/integration/mozilla-inbound/rev/9a449959c891 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bd9c8415a4c
Assignee | ||
Comment 80•9 years ago
|
||
Strange, the fix for this test was in that push, and it all came up green on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d76547017b3
Flags: needinfo?(jmathies)
Assignee | ||
Comment 81•9 years ago
|
||
Pushed another to try with no changes to confirm things are ok there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f1dd77da52
Assignee | ||
Comment 82•9 years ago
|
||
new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=069dd04e659f changes: - disabled browser_plugin_infolink.js under e10s - filed follow up bug 1160166
Comment 83•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/665696faba46 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e278c065d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/74b64ffa82c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/6546042d8d49 https://hg.mozilla.org/integration/mozilla-inbound/rev/103700d6b355
Assignee | ||
Updated•9 years ago
|
Attachment #8595517 -
Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/665696faba46 https://hg.mozilla.org/mozilla-central/rev/d4e278c065d6 https://hg.mozilla.org/mozilla-central/rev/74b64ffa82c2 https://hg.mozilla.org/mozilla-central/rev/6546042d8d49 https://hg.mozilla.org/mozilla-central/rev/103700d6b355
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla39 → mozilla40
Comment 85•9 years ago
|
||
Used the following build for verification: * http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-04-03-02-10-mozilla-central/ For each OS, I went through the following test case on both e10s and non-e10s modes: * ensured that flash appears blocked under about:addons ** ensure that "Ask to Activate" is being used when flash is being blocked * ensured that the "Nightly has prevented the outdated plugin "Adobe Flash" from running..." message correctly appears at the top of the page * ensured that the "This plugin is vulnerable and should be update" message appears when attempting to play a flash video * ensure that clicking on a blocked video displays the doorhanger that allows you to either "Allow Now" or "Allow and Remember" ** ensure "Allow Now" is working (should only work for the current session) ** ensure "Allow and Remember" is working (should always work, even after closing/re-opening fx) * ensured that "Always Activate" is grayed out via about:addons when flash is being blocked Websites used during testing: - http://ca.ign.com - https://apps.facebook.com/candycrush/ - http://espn.go.com - http://www.espnfc.us OS's Used: * Win 7 x64 Pro - PASSED ** Used the following blocked version of Flash: Flash 16.0.0.296 ** Used the following working version of flash: Flash 17.0.0.169 * Ubuntu 14.04.1 - PASSED ** Used the following blocked version of Flash: Flash 11.2.202.425 ** Used the following working version of flash: Flash 11.2.202.457 * OSX 10.10.3 - PASSED ** Used the following blocked version of Flash: Flash 13.0.0.259 ** Used the following working version of flash: Flash 17.0.0.169 Jim, let me know if this is good enough coverage. Looks like it's working on m-c.
Flags: needinfo?(jmathies)
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•