Remove the hiddenwindow special case in CheckLoadURIWithPrincipal

RESOLVED FIXED in Firefox 55

Status

()

Core
Security
RESOLVED FIXED
3 years ago
8 days ago

People

(Reporter: bz, Assigned: Gijs)

Tracking

({addon-compat})

Trunk
mozilla55
x86
Mac OS X
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Need to fix the social API to stop depending on that hackery first.
Flags: needinfo?(mhammond)
Depends on: 1145472
The long term goal here was always to replace the frameworker with real workers - bug 898706.  Bug 936302 seems to be the main blocker here.  Sadly it seems that at least 4 social providers (pocket, mixi, wiebo, cliqz) depend on these workers for their social integration.
Flags: needinfo?(mhammond)
(Assignee)

Comment 2

a year ago
The dep got fixed, so presumably we can do this now? Try push on OSX and linux64 to see if I'm right:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=78e2040b5043
(Assignee)

Comment 3

a year ago
(In reply to :Gijs Kruitbosch from comment #2)
> The dep got fixed, so presumably we can do this now? Try push on OSX and
> linux64 to see if I'm right:
> 
> remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=78e2040b5043

Well, so much for that, looks like the tab thumbnail/preview code tries to use it:

13:04:13     INFO -  80 INFO Console message: Security Error: Content at resource://gre-resources/hiddenWindow.html may not load or link to chrome://global/content/mozilla.xhtml.

http://archive.mozilla.org/pub/firefox/try-builds/gijskruitbosch@gmail.com-78e2040b5043d303a7a9e5e815143dea0d533fb9/try-linux64/try_ubuntu64_vm_test-mochitest-e10s-browser-chrome-3-bm118-tests1-linux64-build200.txt.gz
(Assignee)

Updated

a year ago
Depends on: 1288030
(Assignee)

Updated

5 months ago
Depends on: 1351300
(Assignee)

Updated

5 months ago
Depends on: 1338215
(Assignee)

Comment 4

5 months ago
Maaaaybe we can do this after bug 1351300 lands, let's find out:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c9dbdad131caa31459d3700ce3e4c8d12b81633
(Assignee)

Comment 5

5 months ago
(In reply to :Gijs from comment #4)
> Maaaaybe we can do this after bug 1351300 lands, let's find out:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5c9dbdad131caa31459d3700ce3e4c8d12b81633

This looks green enough, patch incoming.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

Comment 8

5 months ago
mozreview-review
Comment on attachment 8852838 [details]
Bug 1145470 - remove hiddenWindow specialcasing from CheckLoadURI code,

https://reviewboard.mozilla.org/r/125004/#review127710

Gijs++
Attachment #8852838 - Flags: review?(bobbyholley) → review+

Comment 9

5 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f62917e98036
remove hiddenWindow specialcasing from CheckLoadURI code, r=bholley

Comment 10

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f62917e98036
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 11

5 months ago
https://dxr.mozilla.org/addons/search?q=hiddendomwindow&=mozilla-central suggests that there's going to be at least some add-on breakage from this. Not everything people do with this window will break, but expecting it to be effectively chrome-privileged *will* break. Triaging all the 12,000 hits doesn't seem feasible.

I'm pretty comfortable with some breakage - webextensions give people other options, and if people are still maintaining XUL/XPCOM-based addons they can use the createWindowlessBrowser() APIs like our own consumers have done. If we find out (probably not until beta, because eh) that this breaks half the world, it should be reasonably straightforward to back this out at that stage, but if at all possible I'd prefer not to do that.

Jorge, how do I get this relnoted for compat ASAP? :-)
Flags: needinfo?(jorge)
Keywords: addon-compat
Looks like this is why uBO isn't working for me today on m-c tip...
Security Error: Content at resource://gre-resources/hiddenWindow.html may not load or link to chrome://ublock0/content/background.html#1.11.4.
(In reply to :Gijs from comment #11)
> Jorge, how do I get this relnoted for compat ASAP? :-)

We're currently working on the Firefox 54 compat comms, so 55 is a few weeks away.

If this causes significant breakage in add-ons, I would ask if this is something that can be put on hold until 57. Many legacy add-ons won't be ported to WebExtensions and those developers won't have much motivation to keep their add-ons working for only a couple of cycles.
Flags: needinfo?(jorge)
(Assignee)

Comment 14

5 months ago
(In reply to Jorge Villalobos [:jorgev] from comment #13)
> (In reply to :Gijs from comment #11)
> > Jorge, how do I get this relnoted for compat ASAP? :-)
> 
> We're currently working on the Firefox 54 compat comms, so 55 is a few weeks
> away.
> 
> If this causes significant breakage in add-ons, I would ask if this is
> something that can be put on hold until 57.

I don't know how large the breakage will be, see comment #11 . Really, only 1 way to find out.

> Many legacy add-ons won't be
> ported to WebExtensions and those developers won't have much motivation to
> keep their add-ons working for only a couple of cycles.

Well, uBO at least already has a webextension in the works, AIUI. From a security sanity perspective, I would like to see this code gone, and the problem will only get worse once I try to also fix bug 1320124 (which is getting close now)... but I defer to bz/bholley as to how important this is (as security hardening / defense in depth) in the grand scheme of things.
(In reply to :Gijs from comment #14)
> (In reply to Jorge Villalobos [:jorgev] from comment #13)
> > (In reply to :Gijs from comment #11)
> > > Jorge, how do I get this relnoted for compat ASAP? :-)
> > 
> > We're currently working on the Firefox 54 compat comms, so 55 is a few weeks
> > away.
> > 
> > If this causes significant breakage in add-ons, I would ask if this is
> > something that can be put on hold until 57.
> 
> I don't know how large the breakage will be, see comment #11 . Really, only
> 1 way to find out.
> 
> > Many legacy add-ons won't be
> > ported to WebExtensions and those developers won't have much motivation to
> > keep their add-ons working for only a couple of cycles.
> 
> Well, uBO at least already has a webextension in the works, AIUI. From a
> security sanity perspective, I would like to see this code gone, and the
> problem will only get worse once I try to also fix bug 1320124 (which is
> getting close now)... but I defer to bz/bholley as to how important this is
> (as security hardening / defense in depth) in the grand scheme of things.

I would like to see this code go, but I don't think it's worth rocking the boat over it before XPCOM addons are gone for good, given that's only a few months away. So I agree with jorge's assessment here, assuming we do think there's a reasonable risk that this will break addons.
(Assignee)

Updated

5 months ago
Depends on: 1352513
Depends on: 1352866
This breaks Forecastfox (fix version) which is a very popular extension.
Depends on: 1352910
(Assignee)

Updated

5 months ago
No longer depends on: 1352866

Updated

5 months ago
Depends on: 1353488
No longer depends on: 1353488
Depends on: 1352741

Comment 17

8 days ago
IMHO the bug 1145470 fixed wrong currently, because for the hidden window should be "allowed to load URI no matter what.", see the source code (https://hg.mozilla.org/mozilla-central/annotate/09ee763947c3/caps/nsScriptSecurityManager.cpp#l909).
But currently it occurs only if "security.allow_chrome_frames_inside_content" set to true.
I'm not sure that is good idea and possibly this exception for `sourceSpec.EqualsLiteral("resource://gre-resources/hiddenWindow.html")` should take place regardless of value `sCanLoadChromeInContent` (which is the value of security.allow_chrome_frames_inside_content).

Thus ATM the only way to allow such (very many) plugings in ffox >= 55 is to set (in about:config): security.allow_chrome_frames_inside_content = true.
(In addition to `browser.tabs.remote.autostart = false` for plugins not compatible with multi-processing).

If it wanted so, then why this preference (security.allow_chrome_frames_inside_content) was set to "false" per default?
I mean why EACH new firefox version should be ALWAYS incompatible?

@Mozilla: how long you will continue to scoff the developers of addons? Almost each new version of firefox breaks the compatibility of many addons (if a bit larger as "Hello world").
The only reason, why I (and many other) still don't switched to chromium and co, is the Ubiquity plugin, but it will be always more complex to maintain it (as regards firefox upgrade), just take a look https://bitbucket.org/satyr/ubiquity/issues/63/ubiquity-and-electrolysis
(Assignee)

Comment 18

8 days ago
(In reply to sebres from comment #17)

The previous default was a security risk, and this is part of the reason why it is now turned off by default. It's not reasonable for the majority of users to have a less secure browser so that a minority of users can run add-ons that are out of date.

The pref wasn't originally planned, and the only reason it was added was so we could react if a *lot* of add-ons broke. But during beta I haven't heard or seen any complaints, and so we shipped with the pref turned off.

> Thus ATM the only way to allow such (very many) plugings in ffox >= 55 is to
> set (in about:config): security.allow_chrome_frames_inside_content = true.

Which "very many" plugins does this affect? Can you provide a list? The ticket you linked to doesn't mention any other add-ons besides ubiquity.

> (In addition to `browser.tabs.remote.autostart = false` for plugins not
> compatible with multi-processing).

Multi-processing (to a very large degree) and this fix (to a much smaller degree) both contribute to a more secure version of Firefox. Turning both of them off deliberately to run legacy code isn't a configuration we will invest a lot of time to support. The add-ons in question are all going to break completely with the release of Firefox 57, see also e.g. https://blog.mozilla.org/addons/2017/08/10/upcoming-changes-compatibility/ . We have been making quite a lot of noise about this for a while now.

> If it wanted so, then why this preference
> (security.allow_chrome_frames_inside_content) was set to "false" per default?
> I mean why EACH new firefox version should be ALWAYS incompatible?

This isn't my experience of updating Firefox, nor is it that of most users. But ironically, part of how we are making it easier for add-ons to remain compatible with future versions of Firefox is by the move to webextensions - a move which these same add-ons that you're having issues with are apparently not currently making. WebExtensions provide more stable APIs that are less "close to the metal" and therefore more stable in the face of internal changes in Firefox itself.

> Almost each new version of firefox breaks the compatibility of many addons
> (if a bit larger as "Hello world").
> The only reason, why I (and many other) still don't switched to chromium and
> co, is the Ubiquity plugin, but it will be always more complex to maintain
> it (as regards firefox upgrade), just take a look
> https://bitbucket.org/satyr/ubiquity/issues/63/ubiquity-and-electrolysis

Feel free to have a look at the fix I provided for uBlock Origin, where breakage was reported pretty much immediately after this change was made on nightly: https://github.com/gorhill/uBlock/pull/2498/files . Ultimately though, to keep using it in Firefox 57 and beyond, you'll need to convert it to a webextension. https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Porting_a_legacy_Firefox_add-on can help you with this.

Comment 19

8 days ago
@Gijs Thx for you answer.

Just BTW...
> you'll need to convert it to a webextension

Ubiquity is an overlay extension that actively interacts with the page content (can not only get the page-content and properties, but even modify it, e. g. "translate", etc).

A lot of changes is necessary by switch to the webextension-technology.
Unfortunately some things (needed by Ubiquity) are not possibly at all (resp. not yet available) in API for webextension or just not clear how the back-porting may be done at some place (too few documentation for my taste, I meant not the "Hello world" cases).
Another thing that stops myself to make the porting of Ubiquity to webextension, is the thought "What occurs by release of firefox v.70? New Webextension v.2, that will be AGAIN totally incompatible technology?".
You need to log in before you can comment on or make changes to this bug.