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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
There's a race which is causing at least some of my browser-element randomorange.  Patch in a moment.
(Reporter)

Comment 1

5 years ago
Created attachment 640229 [details] [diff] [review]
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.
(Reporter)

Comment 2

5 years ago
Created attachment 640231 [details] [diff] [review]
Part 2, v1: Fix race condition whereby BrowserElementParent.js can be loaded /after/ BrowserElementChild.js, for popup windows in <iframe mozbrowser>.
(Reporter)

Updated

5 years ago
Attachment #640229 - Flags: review?(bugs)
(Reporter)

Updated

5 years ago
Attachment #640231 - Flags: review?(bugs)
(Reporter)

Comment 3

5 years ago
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-
(Reporter)

Comment 7

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

EnsureMessageManager doesn't check mRemoteBrowserShown...
(Reporter)

Comment 8

5 years ago
> 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+
(Reporter)

Updated

5 years ago
Attachment #640229 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Assignee)

Updated

4 years ago
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.