Observers no longer called when loading overlay

RESOLVED FIXED in Firefox 27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mvbrocato, Assigned: bholley)

Tracking

({regression})

24 Branch
mozilla28
x86_64
Windows 7
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 wontfix, firefox26- wontfix, firefox27- fixed, firefox28- fixed, firefox-esr2427+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 824773 [details]
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.
(Reporter)

Comment 1

5 years ago
Created attachment 824774 [details]
overlay.jsp

Comment 2

5 years ago
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
status-firefox25: --- → affected
status-firefox-esr24: --- → affected
tracking-firefox26: --- → ?
tracking-firefox27: --- → ?
tracking-firefox28: --- → ?
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)
(Assignee)

Comment 4

5 years ago
(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....
(Assignee)

Comment 6

5 years ago
(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?
(Assignee)

Comment 8

5 years ago
(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?
(Assignee)

Comment 10

5 years ago
(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().
(Assignee)

Comment 12

5 years ago
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?
(Assignee)

Comment 14

5 years ago
(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.
status-firefox26: --- → affected
status-firefox27: --- → affected
tracking-firefox26: ? → -
tracking-firefox27: ? → -
tracking-firefox28: ? → -
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 17

5 years ago
Created attachment 832332 [details] [diff] [review]
Exempt Remote XUL from CanCreateWrapper checks. v1
Attachment #832332 - Flags: review?(bzbarsky)
(Assignee)

Comment 18

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=4ab547ba5dca
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+
(Assignee)

Comment 21

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 943152
https://hg.mozilla.org/mozilla-central/rev/997ec8454c14
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Duplicate of this bug: 946764
(Assignee)

Comment 26

5 years ago
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+

Updated

5 years ago
tracking-firefox-esr24: --- → ?
https://hg.mozilla.org/releases/mozilla-aurora/rev/344da41d22a3
Assignee: nobody → bobbyholley+bmo
status-firefox25: affected → wontfix
status-firefox26: affected → wontfix
status-firefox27: affected → fixed
status-firefox28: --- → fixed
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
status-firefox27: fixed → affected
Flags: needinfo?(bobbyholley+bmo)
(Assignee)

Comment 30

5 years ago
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)
(Assignee)

Updated

5 years ago
Whiteboard: [uplift needs dependent bug]
Now with more bug 943152 :)

https://hg.mozilla.org/releases/mozilla-aurora/rev/eec4eefa594c
status-firefox27: affected → fixed
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-]
tracking-firefox-esr24: ? → 27+
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
status-firefox-esr24: fixed → affected
(Assignee)

Comment 36

5 years ago
https://hg.mozilla.org/releases/mozilla-esr24/rev/98eff118eff1
status-firefox-esr24: affected → fixed
That did it, thanks :)
You need to log in before you can comment on or make changes to this bug.