DocShell shouldn't have any child when setting userContextId

RESOLVED FIXED in Firefox 49

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

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

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)

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]

Updated

3 years ago
Priority: -- → P1

Updated

3 years ago
Status: NEW → ASSIGNED
Created attachment 8749515 [details] [diff] [review]
Patch.
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

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e7bfb11e947d
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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 → ---
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.
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

Comment 24

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