DocGroups for documents with a null principal shouldn't match

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
5 months 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)

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.)
Duplicate of this bug: 1334320
Duplicate of this bug: 1334322
The fundamental issue here is that DocGroup::GetKey is trying to simulate nsIPrincipal::GetBaseDomain() and just doing a very good job at it.
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.
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.
Depends on: 1334047
See Also: 1334047
(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 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-
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)
Attachment #8831155 - Attachment is obsolete: true
Attachment #8831155 - Flags: review?(wmccloskey)
Attachment #8831207 - Flags: review?(wmccloskey) → review+
Depends on: 1335526
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.
Depends on: 1336213
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.
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
https://hg.mozilla.org/mozilla-central/rev/70b6cd2dc27f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.