DocGroups for documents with a null principal shouldn't match

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Right now when we encounter the null principal we throw up our hands and compute an empty string, therefore all docs with a null principal end up in the same DocGroup which is the exact opposite of the semantics that we want (they should all end up in their own unique DocGroup.)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1334320
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1334322
(Assignee)

Comment 3

2 years ago
The fundamental issue here is that DocGroup::GetKey is trying to simulate nsIPrincipal::GetBaseDomain() and just doing a very good job at it.
(Assignee)

Comment 4

2 years ago
Also I think defaulting to an empty string is a bad idea since that will put the documents that this code fails for all into the same docgroup.  I think it's better to be more conservative and assign each such document to a unique docgroup.
(Assignee)

Comment 5

2 years ago
It turns out that after fixing this bug, and bug 1334047, there are now more test failures that were masked in the same way bug 1334047 was masked, so I need to fix them all first before landing this.
(Assignee)

Updated

2 years ago
Depends on: 1334047
See Also: bug 1334047
(Assignee)

Comment 6

2 years ago
Created attachment 8831155 [details] [diff] [review]
Fix the docgroup key computation to account for things such as null principals, URLs with IP address hostnames, etc.
Attachment #8831155 - Flags: review?(wmccloskey)

Comment 7

2 years ago
(In reply to :Ehsan Akhgari from comment #4)
> Also I think defaulting to an empty string is a bad idea since that will put
> the documents that this code fails for all into the same docgroup.  I think
> it's better to be more conservative and assign each such document to a
> unique docgroup.

Putting multiple documents in the same docgroup _is_ the conservative option. Putting them in different docgroups mandates that they cannot synchronously communciate. For example, all chrome documents go into the same docgroup, because they can all communicate with eachother synchronously. 

You have to be careful when creating new docgroups that they cannot communicate synchronously.
Flags: needinfo?(ehsan)

Comment 8

2 years ago
Comment on attachment 8831155 [details] [diff] [review]
Fix the docgroup key computation to account for things such as null principals, URLs with IP address hostnames, etc.

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

::: dom/base/DocGroup.cpp
@@ +23,5 @@
>    if (NS_FAILED(rv)) {
> +    // We don't really know what to do here.  But we should be conservative,
> +    // otherwise it would be possible to reorder two events incorrectly in the
> +    // future if we interrupt at the DocGroup level, so to be safe,
> +    // generate a unique key.

This is wrong - it would be possible to reorder them incorrectly in the future if we use a unique key, but would not be possible with them using the same key!
Attachment #8831155 - Flags: feedback-
(Assignee)

Comment 9

2 years ago
Hmm, yes I think you're right.  I don't know why I was thinking in reverse terms last night, I blame a long debugging session!
Flags: needinfo?(ehsan)
(Assignee)

Comment 10

2 years ago
Created attachment 8831207 [details] [diff] [review]
Fix the docgroup key computation to account for things such as null principals, URLs with IP address hostnames, etc.
Attachment #8831207 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Attachment #8831155 - Attachment is obsolete: true
Attachment #8831155 - Flags: review?(wmccloskey)
Attachment #8831207 - Flags: review?(wmccloskey) → review+
(Assignee)

Updated

2 years ago
Depends on: 1335526
(Assignee)

Comment 11

2 years ago
It seems that fixing bug 1335526 fixes many of the assertion failures I was catching on try (see bug 1334047 comment 6), but I want to do another round to be sure.
(Assignee)

Updated

2 years ago
Depends on: 1336213
(Assignee)

Comment 12

2 years ago
I had a bug in this patch where I was returning NS_OK from DocGroup::GetKey() unconditionally.  This caused a shutdown crash where the call to nsIPrincipal::GetBaseDomain() would fail because XPCOM services are dead at that time, but the success code caused us to mistakenly compare the docgroup ID we had stored against an empty string caused by the failure above.  Returning rv fixes it.  I'll fix this when landing.

Comment 13

2 years ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70b6cd2dc27f
Fix the docgroup key computation to account for things such as null principals, URLs with IP address hostnames, etc.; r=billm

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70b6cd2dc27f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.