Closed Bug 772076 Opened 9 years ago Closed 9 years ago

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

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: justin.lebar+bug, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

There's a race which is causing at least some of my browser-element randomorange.  Patch in a moment.
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.
Attachment #640229 - Flags: review?(bugs)
Attachment #640231 - Flags: review?(bugs)
This bug blocks the world, because all of these bugs have tests which fail intermittently without this fix.
So part 1 isn't needed?
Could you explain why part 2 is needed.
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 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.
Attachment #640229 - Flags: review?(bugs) → review-
> EnsureMessageManager couldn't actually
> properly initialize message manager because mRemoteBrowserShown is false.

EnsureMessageManager doesn't check mRemoteBrowserShown...
> 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...
Attachment #640231 - Flags: review?(bugs) → review+
Attachment #640229 - Attachment is obsolete: true
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.
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
https://hg.mozilla.org/mozilla-central/rev/3ac72d604773
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.