Remove the hiddenwindow special case in CheckLoadURIWithPrincipal

RESOLVED FIXED in Firefox 55

Status

()

Core
Security
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: bz, Assigned: Gijs)

Tracking

(Depends on: 1 bug, {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

11 months 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

11 months 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

11 months ago
Depends on: 1288030
(Assignee)

Updated

3 months ago
Depends on: 1351300
(Assignee)

Updated

3 months ago
Depends on: 1338215
(Assignee)

Comment 4

3 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

3 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

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

Comment 8

3 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

3 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

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

Comment 11

3 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

3 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

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

Updated

3 months ago
No longer depends on: 1352866

Updated

3 months ago
Depends on: 1353488
No longer depends on: 1353488
Depends on: 1352741
You need to log in before you can comment on or make changes to this bug.