932906 - Observers no longer called when loading overlay

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[mvbrocato]

Attachment: 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.

Comment 1

[mvbrocato]

Attachment: overlay.jsp

Comment 2

[Alice0775 White]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Sounds like some of those callers were relying on CallerTypeIsJavaScript() testing false and hence security checks being skipped, no?

Comment 4

[Bobby Holley (:bholley)]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Still need to fix it, though....

Comment 6

[Bobby Holley (:bholley)]

(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?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, what's failing exactly?  Are we running security checks when we actually shouldn't somehow?

Comment 8

[Bobby Holley (:bholley)]

(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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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?

Comment 10

[Bobby Holley (:bholley)]

(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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

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().

Comment 12

[Bobby Holley (:bholley)]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Bobby Holley (:bholley)]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hmm.  Can we do that easily?  If so, that might be the way to go.

Comment 16

[bhavana bajaj [:bajaj]]

Hasn't been an issue post-release of FF24. We'd take a low risk uplift, but no need to track.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Exempt Remote XUL from CanCreateWrapper checks. v1

Comment 18

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4ab547ba5dca

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 832332 [details] [diff] [review]
Exempt Remote XUL from CanCreateWrapper checks. v1

r=me

Comment 20

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1ff51aa5aca8

Comment 21

[Bobby Holley (:bholley)]

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

Comment 22

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=4e1cd6c700ea

Comment 23

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/997ec8454c14

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/997ec8454c14

Comment 26

[Bobby Holley (:bholley)]

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 27

[bhavana bajaj [:bajaj]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/344da41d22a3

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Bobby Holley (:bholley)]

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?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

A whiteboard comment is definitely helpful, yes.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Now with more bug 943152 :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/eec4eefa594c

Comment 33

[u279076]

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.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/a230e41d7a1a

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Bug 943152 had to be backed out from esr24, so I backed this out too.
https://hg.mozilla.org/releases/mozilla-esr24/rev/d9c242f679ee

Comment 36

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-esr24/rev/98eff118eff1

Comment 37

[Bobby Holley (:bholley)]

Followup bustage fix:

https://hg.mozilla.org/releases/mozilla-esr24/rev/38de81ccdf0b

Comment 38

[Ryan VanderMeulen [:RyanVM]]

That did it, thanks :)