772076 - [BrowserAPI] Fix race condition in loading BrowserElementParent.js for popup windows

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

There's a race which is causing at least some of my browser-element randomorange.  Patch in a moment.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1: Show() remote frame /after/ loading BrowserElementParent.js, to prevent a race.

I'm not sure this change is actually necessary.  It /seems/ necessary, but this change didn't fix any oranges on my machine.  Maybe it'll make a difference on try.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1: Fix race condition whereby BrowserElementParent.js can be loaded /after/ BrowserElementChild.js, for popup windows in <iframe mozbrowser>.

Comment 3

[Justin Lebar (not reading bugmail)]

This bug blocks the world, because all of these bugs have tests which fail intermittently without this fix.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

So part 1 isn't needed?
Could you explain why part 2 is needed.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 640229 [details] [diff] [review]
Part 1, v1: Show() remote frame /after/ loading BrowserElementParent.js, to prevent a race.

This looks regression prone. EnsureMessageManager couldn't actually
properly initialize message manager because mRemoteBrowserShown is false.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 640229 [details] [diff] [review]
Part 1, v1: Show() remote frame /after/ loading BrowserElementParent.js, to prevent a race.

So, is this isn't needed, we shouldn't take this.

Comment 7

[Justin Lebar (not reading bugmail)]

> EnsureMessageManager couldn't actually
> properly initialize message manager because mRemoteBrowserShown is false.

EnsureMessageManager doesn't check mRemoteBrowserShown...

Comment 8

[Justin Lebar (not reading bugmail)]

> So, if this isn't needed, we shouldn't take this.

It's an obvious race condition: The child loads BrowserElementChild.js when it receives Show(), and the parent must be initialized before the child.  Even if I don't have a failing test, it's still wrong...

Comment 9

[Justin Lebar (not reading bugmail)]

Smaug explained over IRC how part 1 isn't actually a race condition.  Although the child receives its Show() before BrowserElementParent.js is initialized, we won't process any messages from the child until we return to the event loop, so it's OK to initialize BrowserElementParent after we Show() the child.

Comment 10

[Justin Lebar (not reading bugmail)]

There was some orange in my try push, but it's not clear whether it's from part 1 (which I'm not checking in), part 2, or other patches in my queue.  I think the orange is likely unrelated, so I pushed part 2.  We'll see if I was right.  :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac72d604773

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ac72d604773