1250033 - DocShell shouldn't have any child when setting userContextId

Status: RESOLVED FIXED

(Core :: Security, defect, P1)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

When I was working on Bug 1227861 we found that docshell already has children when setting userContextId, which should be a bug
See https://bugzilla.mozilla.org/show_bug.cgi?id=1227861#c53

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=359a4e1a74b6

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4579209f0eb3&selectedJob=20400428

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8749515 [details] [diff] [review]
Patch.

Review of attachment 8749515 [details] [diff] [review]:
-----------------------------------------------------------------

We should also assert that mContentViewer is null. Is there a followup bug on that?

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> Comment on attachment 8749515 [details] [diff] [review]
> Patch.
> 
> Review of attachment 8749515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should also assert that mContentViewer is null. Is there a followup bug
> on that?

Thanks for the 'early' review.
Bug 1250063, I am still working on this as it involves code related to nsWindowWatcher.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7bfb11e947d

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The landing of this patch caused a constant crash in our firefox-ui-tests as filed as bug 1271212.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The try build from comment 3 also shows this crash:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4579209f0eb3&selectedJob=20401993&filter-tier=1&filter-tier=2&filter-tier=3

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7bfb11e947d

Comment 10

[Carsten Book [:Tomcat]]

backing out in case bug 1271212 would affect also nightly users

Comment 11

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/7adbdf0fce9a

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this also affect 48? 
Marking 49 as affected since this landed and then backed out.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this problem go back fairly far in Firefox versions? How important is it to fix? 
Tracking for 49 until I understand the issue better.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What test was failing? It seems quite likely that the test is doing something that's insecure. Hopefully it's just the test itself that's doing the insecure thing, and not product code.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The test which was failing is a Marionette test from the firefox-ui-tests suite:

https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/tests/functional/security/test_no_certificate.py?from=test_no_certificate.py

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Log details can also be found here:
https://public-artifacts.taskcluster.net/ezYec3LDRQmhvIeF88GPcw/0/public/logs/live_backing.log

Comment 18

[Tanvi Vyas[:tanvi]]

Is this an OriginAttributes but, or usercontextId specific?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This primarily affects OriginAttributes. But since userContextId is built on top of OriginAttributes it'll affect userContextIds as well. So the problem here isn't that userContextId specifically are used the wrong way, but that all OriginAttributes, userContextId included, are used the wrong way.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

The patch from Bug 1250033 should fix the assertion.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=caf6351ceb58

Comment 21

[Ethan Tseng [:ethan]]

Corrected a typo in comment 20 (just to avoid confusion).

The patch from Bug 1250063 should fix the assertion.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=caf6351ceb58

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9c08fa524a&filter-tier=1&filter-tier=2&filter-tier=3

Firefox-UI-functional-e10s looks okay.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abf706cbf6a2

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/abf706cbf6a2