Closed Bug 932906 Opened 7 years ago Closed 7 years ago
Observers no longer called when loading overlay
790 bytes, application/xml
254 bytes, text/plain
4.36 KB, patch
|Details | Diff | Splinter Review|
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36 Steps to reproduce: As of Firefox 24, we've noticed that observers are not notified when loading an overlay using document.loadOverlay. Note that this is a remote XUL site that I am testing from localhost. We've been using the remote XUL manager add on for some time to enable remote XUL for our domains. See the attached files for a simple example that illustrates the problem Actual results: The overlay is loaded, but the observe() method is not called. The error console shows "Error: Permission denied for <http://localhost> to create wrapper for object of class UnnamedClass" Expected results: The observe method should have been called.
Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/8f9ba85eb61c Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130605 Firefox/24.0 ID:20130605001414 Bad: http://hg.mozilla.org/mozilla-central/rev/b925d7cdd09a Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130605 Firefox/24.0 ID:20130605034215 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8f9ba85eb61c&tochange=b925d7cdd09a Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/3378602c5b0c Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604195414 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/888bbc708061 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130604 Firefox/24.0 ID:20130604195816 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3378602c5b0c&tochange=888bbc708061 In local build: Last Good: f6eb97809d91 First Bad: 2a2ad2ce8a4c Regressed by: 2a2ad2ce8a4c Bobby Holley — Bug 877478 - Move all consumers of GetAppropriateSecurityManager to GetDefaultSecurityManager and rm the former. r=mrbkap
Still need to fix it, though....
(In reply to Boris Zbarsky [:bz] from comment #5) > Still need to fix it, though.... Well, how exactly? Do we want just disable these security checks for remote XUL domains?
Well, what's failing exactly? Are we running security checks when we actually shouldn't somehow?
(In reply to Boris Zbarsky [:bz] from comment #7) > Well, what's failing exactly? Are we running security checks when we > actually shouldn't somehow? The remote XUL page defines an observer and passes it to document.loadOverlay. When the observer is invoked, it's invoked with various XPCOM-y argument types (without the DOM bit in classinfo) for which we're never supposed to allow reflectors in unprivileged scopes. So we fail.
In particular nsIURI, I guess? That's moderately annoying. So in the old world we'd pass in an nsIURI object to the unprivileged observer but it just couldn't do anything with it?
(In reply to Boris Zbarsky [:bz] from comment #9) > So in the old world we'd pass in an nsIURI object to the unprivileged > observer but it just couldn't do anything with it? Why couldn't it do anything with it?
I thought XPConnect did security checks on every access. And nsScriptSecurityManager::CheckPropertyAccessImpl when it fails to find a security policy will default to SCRIPT_SECURITY_NO_ACCESS if there is no classinfo claiming IsDOMClass().
Oh yeah, those. We don't rely on them anymore, so I'm not convinced they work reliably. I was able to remove them in my patches for bug 913734 whilst only changing one copy/paste test.
Anyway. _If_ that used to be the case, how about we check whether caller is chrome in LoadOverlay(), and if not set a flag on our observer entry that will make us just pass a null URI to the observer? Seems like it would get us to a state that's no worth than before, right?
(In reply to Boris Zbarsky [:bz] from comment #13) > Anyway. _If_ that used to be the case, how about we check whether caller is > chrome in LoadOverlay(), and if not set a flag on our observer entry that > will make us just pass a null URI to the observer? Seems like it would get > us to a state that's no worth than before, right? Well, it's kind of gross, and there's no guarantee that this is the only case that people are going to run into. It's kind of a crapshoot. The other option is to disable CanCreateWrapper checks (or at least some subset of them) for remote XUL.
Hmm. Can we do that easily? If so, that might be the way to go.
Hasn't been an issue post-release of FF24. We'd take a low risk uplift, but no need to track.
Comment on attachment 832332 [details] [diff] [review] Exempt Remote XUL from CanCreateWrapper checks. v1 r=me
Attachment #832332 - Flags: review?(bzbarsky) → review+
Hm, one failure that's consistent across all platforms on try, but doesn't reproduce locally. Let's push some diagnostics to see what's going on: https://tbpl.mozilla.org/?tree=Try&rev=f62ea388193d For reference, this is the output of the diagnostics in my (successful) local run: > 1:00.41 Entering CCNotPresent... > 1:00.43 Defined. ToString()ing... > 1:00.43 [object Object] > 1:00.44 Our origin: http://mochi.test:8888 > 1:00.44 Pref value: true
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Comment on attachment 832332 [details] [diff] [review] Exempt Remote XUL from CanCreateWrapper checks. v1 We should get this on aurora (soon to be beta) and esr24. ESR24 is especially important, I think, because that's where a lot of our remote XUL consumers are, and they're all switching over from esr17. We should get this into tuesday's release if possible, but I don't know how possible that is. This approval request also covers the patch in bug 943152, which is an automation patch that prevents the tests for this bug from turning the tree orange. [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 877478 User impact if declined: Breakage in remote XUL pages Testing completed (on m-c, etc.): Just landed to m-c. Risk to taking this patch (and alternatives if risky): Low-risk. String or IDL/UUID changes made by this patch: None
Comment on attachment 832332 [details] [diff] [review] Exempt Remote XUL from CanCreateWrapper checks. v1 Approving on aurora at this time as its a low risk uplift. For current esr, we have already gone to build, so unfortunately this will have to land for the next round
Attachment #832332 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out for mochitest-other failures. https://hg.mozilla.org/releases/mozilla-aurora/rev/fe1898c72e2e https://tbpl.mozilla.org/php/getParsedLog.php?id=31595085&tree=Mozilla-Aurora
You also need the patch from bug 943152, as noted in comment 26. Is there a way I should make that more clear? Whiteboard or something?
Flags: needinfo?(bobbyholley+bmo) → needinfo?(ryanvm)
A whiteboard comment is definitely helpful, yes.
Given in-testsuite coverage this fix will not be manually verified by QA. If you believe this warrants extra QA attention please nominate for testing by removing this whiteboard tag and adding the verifyme keyword. Please also provide any details you have that may inform our testing.
Attachment #832332 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Bug 943152 had to be backed out from esr24, so I backed this out too. https://hg.mozilla.org/releases/mozilla-esr24/rev/d9c242f679ee
Followup bustage fix: https://hg.mozilla.org/releases/mozilla-esr24/rev/38de81ccdf0b
That did it, thanks :)
You need to log in before you can comment on or make changes to this bug.