47 bytes, text/x-phabricator-request
|Details | Review|
(from a conversation with Nika) It would be nice to have a pref to make privileged code access to cross-origin frames fail somehow, either throwing or returning null. By toggling this pref we can see what will fail/break in our code that needs to be fixed for Fission. Note 1: ideally this wouldn't break parent code accessing non-e10s tabs Note 2: I've been tying some other fission-simulation behavior to the browser.fission.simulate pref, but we can use another one for more granularity, if desired
Another point that was mentioned by Nika in the meeting: even though this will start as a pref, the end goal is to make this the default enforced behavior, in order to prevent people from writing different codepaths for same-origin and cross-origin frames.
I've offered to mentor Felipe through these changes after I'm back. It would be awesome to have this for the all-hands.
Assignee: nobody → felipc
Component: DOM → DOM: Core & HTML
Priority: P3 → P2
Summary: Add a pref to to return null on cross-origin frames lookups for chrome code → Add a pref to to return null on chrome->content subframe access
The pref dom.chrome_frame_access.enabled will default to true. When false, it will block various methods that chrome code can use to traverse subframes. The initial list is: iframe.contentWindow iframe.contentDocument window.top window.parent window.frames MessageEvent.source More blocks are likely to be added in the future.
A couple of notes about the patch: - I'm not sure if this is the best approach for the indexed getter. I tried to stick to what we discussed and how the other cases behaved: It doesn't change window.frames.length, but window.frames[i] will return null for the frames that are not also privileged. A different approach would be to just make window.frames[i] * always * return null (independent of the object that would be returned), and make window.frames.length return 0. This sounds nicer and will likely break less code (one breakage can be seen in the devtools console part of this patch that I had to fix in order to continue using devtools). The downside is that it wouldn't follow the "still allow chrome to chrome access" part. - as far as I can tell, window.frames["named getter"] already doesn't work for chrome code trying to get a named iframe from content. I had added code to WindowNamedPropertiesHandler.cpp for that but it turned out to be unecessary. - I named the pref to something that defaults to true, because otherwise the double negative was getting to confusing (originally we had talked about dom.strict_chrome_frame_access.enabled = false)
Status: NEW → ASSIGNED
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/82ca028b0524 Add a pref to block chrome code from accessing content subframes. r=nika
Backed out changeset 82ca028b0524 (Bug 1503984) for StaticPrefList failures. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=82ca028b05248d99b0e4638cae0c0b7d99ced797&selectedJob=217752095 Backout link: https://hg.mozilla.org/integration/autoland/rev/1ec6e2d441b1dcc104d6a8807462511f1b8eb122 Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217752095&repo=autoland&lineNumber=1138 [task 2018-12-18T20:04:07.044Z] 20:04:07 INFO - web-platform-tests CT,, [task 2018-12-18T20:04:07.045Z] 20:04:07 INFO - [task 2018-12-18T20:04:07.046Z] 20:04:07 INFO - Application command: /builds/worker/workspace/build/application/firefox/firefox --marionette about:blank -profile /tmp/tmpsvVdPz.mozrunner [task 2018-12-18T20:04:07.050Z] 20:04:07 INFO - Starting runner [task 2018-12-18T20:04:07.077Z] 20:04:07 INFO - PID 912 | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpsvVdPz.mozrunner/runtests_leaks_857.log [task 2018-12-18T20:04:07.598Z] 20:04:07 INFO - PID 912 | Assertion failure: IsAtomic<bool>::value || NS_IsMainThread() (Non-atomic static pref 'dom.chrome_frame_access.enabled' being accessed on background thread), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StaticPrefList.h:295 [task 2018-12-18T20:04:58.017Z] 20:04:58 INFO - PID 912 | #01: nsGlobalWindowInner::AllowChromeFrameAccess(JSContext*, JSObject*) [dom/base/nsGlobalWindowInner.cpp:2889] [task 2018-12-18T20:04:58.017Z] 20:04:58 INFO - [task 2018-12-18T20:04:58.018Z] 20:04:58 INFO - PID 912 | #02: mozilla::dom::PrefableDisablers::isEnabled(JSContext*, JS::Handle<JSObject*>) const [dom/bindings/DOMJSClass.h:132] [task 2018-12-18T20:04:58.018Z] 20:04:58 INFO - [task 2018-12-18T20:04:58.019Z] 20:04:58 INFO - PID 912 | #03: bool mozilla::dom::DefinePrefable<JSPropertySpec const>(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Prefable<JSPropertySpec const> const*) [dom/bindings/BindingUtils.cpp:653]
The failure was about accessing this static pref off the main thread. Is the right thing to do to just change it to a RelaxedAtomicBool? (The comment in PrefableDisablers  say this is only related to things with a "Pref" annotation, but I assume it's also true for things with a "Func" annotation, right?). I'm asking because I didn't make changes to any property with a "Pref" annotation  https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/dom/bindings/DOMJSClass.h#118
Flags: needinfo?(felipc) → needinfo?(nika)
(In reply to :Felipe Gomes (needinfo me!) from comment #7) > The failure was about accessing this static pref off the main thread. Is the > right thing to do to just change it to a RelaxedAtomicBool? > > (The comment in PrefableDisablers  say this is only related to things > with a "Pref" annotation, but I assume it's also true for things with a > "Func" annotation, right?). I'm asking because I didn't make changes to any > property with a "Pref" annotation > > >  > https://searchfox.org/mozilla-central/rev/ > 13788edbabb04d004e4a1ceff41d4de68a8320a2/dom/bindings/DOMJSClass.h#118 I was initially surprised that this method was called off-main-thread at all, but I suppose you're using it as a function to disable the `source` property on `MessageEvent`. We probably don't want to disable `MessageEvent::source` if the source is not a window object, but rather a port. I think the easiest solution here is to not disable MessageEvent's properties in this first version, and revisit what we want to do there later.
Yeah, that surprised me too. I removed the MessageEvent parts from the code and tests, reverted back to a normal `bool` StaticPref and sent it to tryserver to make sure nothing else is unexpectedly touching it outside of the main thread. Let's see how it goes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e869b2aa7a2ce24ec9c050d7cb1a8b078025d45
It was indeed the MessageEvent.source that caused that
Flags: needinfo?(nika) → needinfo?(bzbarsky)
Whiteboard: [2/14] per comment 11, resolving issues after rebasing the patch
Whiteboard: [2/14] per comment 11, resolving issues after rebasing the patch → [2/22] patch r+, ready to land
Whiteboard: [2/22] patch r+, ready to land → [2/22] patch needs to be rewritten with different approach
Status: ASSIGNED → RESOLVED
Last Resolved: 3 days ago
Resolution: --- → WORKSFORME
Whiteboard: [2/22] patch needs to be rewritten with different approach
You need to log in before you can comment on or make changes to this bug.