DocShell shouldn't have any child when setting userContextId

RESOLVED FIXED in Firefox 49

Status

()

Core
Security
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: allstars, Assigned: allstars)

Tracking

(Blocks: 1 bug)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 affected, firefox49+ fixed)

Details

(Whiteboard: [userContextId])

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=359a4e1a74b6
Whiteboard: [userContextId]

Updated

2 years ago
Priority: -- → P1

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8749515 [details] [diff] [review]
Patch.
Attachment #8749515 - Flags: review?(jonas)
(Assignee)

Comment 3

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4579209f0eb3&selectedJob=20400428
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?
Attachment #8749515 - Flags: review?(jonas) → review+
(Assignee)

Comment 5

2 years ago
(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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7bfb11e947d
The landing of this patch caused a constant crash in our firefox-ui-tests as filed as bug 1271212.
Depends on: 1271212
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7bfb11e947d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
backing out in case bug 1271212 would affect also nightly users
Status: RESOLVED → REOPENED
Flags: needinfo?(allstars.chh)
Resolution: FIXED → ---

Comment 11

2 years ago
Backout:
https://hg.mozilla.org/mozilla-central/rev/7adbdf0fce9a
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1271212
Does this also affect 48? 
Marking 49 as affected since this landed and then backed out.
status-firefox48: --- → affected
status-firefox49: fixed → affected
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.
tracking-firefox49: --- → +
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.
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
Log details can also be found here:
https://public-artifacts.taskcluster.net/ezYec3LDRQmhvIeF88GPcw/0/public/logs/live_backing.log

Comment 18

2 years ago
Is this an OriginAttributes but, or usercontextId specific?
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 hidden (typo)
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
(Assignee)

Comment 22

2 years ago
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

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

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/abf706cbf6a2
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.