Closed Bug 932906 Opened 7 years ago Closed 7 years ago

Observers no longer called when loading overlay

Categories

(Core :: XPConnect, defect)

24 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox25 --- wontfix
firefox26 - wontfix
firefox27 - fixed
firefox28 - fixed
firefox-esr24 27+ fixed

People

(Reporter: mvbrocato, Assigned: bholley)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(3 files)

Attached file main.jsp
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.
Attached file overlay.jsp
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
Status: UNCONFIRMED → NEW
Component: Untriaged → XPConnect
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Sounds like some of those callers were relying on CallerTypeIsJavaScript() testing false and hence security checks being skipped, no?
Flags: needinfo?(bobbyholley+bmo)
(In reply to Boris Zbarsky [:bz] from comment #3)
> Sounds like some of those callers were relying on CallerTypeIsJavaScript()
> testing false and hence security checks being skipped, no?

Yeah, probably. It's only a coincidence that it ever worked, really.
Flags: needinfo?(bobbyholley+bmo)
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.
Flags: needinfo?(bobbyholley+bmo)
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
Depends on: 943152
https://hg.mozilla.org/mozilla-central/rev/997ec8454c14
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Duplicate of this bug: 946764
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
Attachment #832332 - Flags: approval-mozilla-esr24?
Attachment #832332 - Flags: approval-mozilla-aurora?
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+
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.
Flags: needinfo?(ryanvm)
Whiteboard: [uplift needs dependent bug]
Flags: in-testsuite+
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.
Whiteboard: [qa-]
Attachment #832332 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
That did it, thanks :)
You need to log in before you can comment on or make changes to this bug.