Closed Bug 787131 Opened 8 years ago Closed 8 years ago

Move handling of dynamic type attr changes on <xul:browser> out of nsSubDocumentFrame and into nsFrameLoader

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

The goal is for this to work even when the <browser> is display:none.
Depends on: 776928
Blocks: 776928
No longer depends on: 776928
Summary: Move handling of dynamic type attr changes on <xul:browser> our of nsSubDocumentFrame and into nsFrameLoader → Move handling of dynamic type attr changes on <xul:browser> out of nsSubDocumentFrame and into nsFrameLoader
Comment on attachment 656941 [details] [diff] [review]
Move management of primary content state stuff from the nsSubDocumentFrame into the nsFrameLoader, so it works on display:none elements.

> void
> nsFrameLoader::SetOwnerContent(Element* aContent)
> {
>+  if (!aContent && mObservingOwnerContent) {
>+    mObservingOwnerContent = false;
>+    mOwnerContent->RemoveMutationObserver(this);
>+  }
>   mOwnerContent = aContent;

The code above is correct only if !!aContent implies !mObservingOwnerContent,
right?  Otherwise we could leave an observer on the old mOwnerContent.

It happens that the invariant above is satisfied in the instances where
SetOwnerContent is called, but it's not clear that's the only correct way to
call SetOwnerContent.

I guess you're working around the complexity that arises during swapping due to
the possibility of aContent == mOwnerContent?  Still, it's quite abstruse.

Perhaps the easiest way to simplify this would be to
assert(!mObservingOwnerContent) at the top of SetOwnerContent and change the
one relevant call to SetOwnerContent(nullptr) to remove the observer first?

This looks fine to me otherwise, although I have to admit that I don't
understand how this change affects bug 776928.
Attachment #656941 - Flags: review?(justin.lebar+bug) → review+
> The code above is correct only if !!aContent implies !mObservingOwnerContent,
> right? 

No, it's also correct if "!!aContent && mObservingOwnerContent" means we're actually already observing aContent.  Which happens to be true in the swap case, because we call AddTreeItemToTreeOwner before we call SetOwnerContent.  I could try to reorder those, but I was trying to go for a minimally invasive fix.

> Perhaps the easiest way to simplify this would be to
> assert(!mObservingOwnerContent) at the top of SetOwnerContent

Per above, that would assert on swaps.

> although I have to admit that I don't understand how this change affects bug 776928.

With the patch in that bug, we end up with situations where when a tab is coming to foreground its type changes to content-primary in the same function as its style display value, but before the display change is processed and creates a frame.  So we were failing to tell the treeowner about the new content-primary docshell.
> Per above, that would assert on swaps.

Ah, right, because mObservingOwnerContent, at that point, means mObservingMyNewOwnerContent.  :-/

> I could try to reorder those, but I was trying to go for a minimally invasive fix.

Reordering them looks safe to me, and would probably be much saner.
With the reordering:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a014a515052
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/9a014a515052
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.