Last Comment Bug 772076 - [BrowserAPI] Fix race condition in loading BrowserElementParent.js for popup windows
: [BrowserAPI] Fix race condition in loading BrowserElementParent.js for popup ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla16
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: browser-api 764718 769182 769254 770239 771273
  Show dependency treegraph
 
Reported: 2012-07-09 08:05 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-04-04 13:53 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1, v1: Show() remote frame /after/ loading BrowserElementParent.js, to prevent a race. (1.24 KB, patch)
2012-07-09 08:25 PDT, Justin Lebar (not reading bugmail)
bugs: review-
Details | Diff | Review
Part 2, v1: Fix race condition whereby BrowserElementParent.js can be loaded /after/ BrowserElementChild.js, for popup windows in <iframe mozbrowser>. (2.94 KB, patch)
2012-07-09 08:26 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2012-07-09 08:05:55 PDT
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) 2012-07-09 08:25:30 PDT
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.
Comment 2 Justin Lebar (not reading bugmail) 2012-07-09 08:26:34 PDT
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>.
Comment 3 Justin Lebar (not reading bugmail) 2012-07-09 08:35:16 PDT
This bug blocks the world, because all of these bugs have tests which fail intermittently without this fix.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-07-09 09:00:21 PDT
So part 1 isn't needed?
Could you explain why part 2 is needed.
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-07-09 09:01:28 PDT
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] (high review load, please consider other reviewers) 2012-07-09 09:03:37 PDT
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) 2012-07-09 09:03:56 PDT
> EnsureMessageManager couldn't actually
> properly initialize message manager because mRemoteBrowserShown is false.

EnsureMessageManager doesn't check mRemoteBrowserShown...
Comment 8 Justin Lebar (not reading bugmail) 2012-07-09 09:05:09 PDT
> 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) 2012-07-09 10:23:54 PDT
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) 2012-07-09 11:15:45 PDT
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] 2012-07-09 18:03:25 PDT
https://hg.mozilla.org/mozilla-central/rev/3ac72d604773

Note You need to log in before you can comment on or make changes to this bug.