1334281 - DocGroups for documents with a null principal shouldn't match

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

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.)

Comment 3

[(no longer active)]

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

Comment 4

[(no longer active)]

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.

Comment 5

[(no longer active)]

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.

Comment 6

[(no longer active)]

Attachment: Fix the docgroup key computation to account for things such as null principals, URLs with IP address hostnames, etc.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

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!

Comment 9

[(no longer active)]

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!

Comment 10

[(no longer active)]

Attachment: Fix the docgroup key computation to account for things such as null principals, URLs with IP address hostnames, etc.

Comment 11

[(no longer active)]

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.

Comment 12

[(no longer active)]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70b6cd2dc27f