Closed Bug 1250033 Opened 4 years ago Closed 3 years ago

DocShell shouldn't have any child when setting userContextId

Categories

(Core :: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- affected
firefox48 --- affected
firefox49 + fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId])

Attachments

(1 file)

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
Whiteboard: [userContextId]
Priority: -- → P1
Status: NEW → ASSIGNED
Attached patch Patch.Splinter Review
Attachment #8749515 - Flags: review?(jonas)
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+
(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.
The landing of this patch caused a constant crash in our firefox-ui-tests as filed as bug 1271212.
Depends on: 1271212
https://hg.mozilla.org/mozilla-central/rev/e7bfb11e947d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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 → ---
Duplicate of this bug: 1271212
Does this also affect 48? 
Marking 49 as affected since this landed and then backed out.
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.
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.
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.
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
https://hg.mozilla.org/mozilla-central/rev/abf706cbf6a2
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.