Closed
Bug 1162700
Opened 10 years ago
Closed 10 years ago
With e10s, need to initialize app-info sooner in the content process - before graphics is
Categories
(Core :: DOM: Content Processes, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: milan, Assigned: milan)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
5.25 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
When initializing graphics, we need to know the version we're running, so that we can interpret the blocklist correctly (see bug 1162530.) That works in the parent process, but for the child process, app-info is initialized too late, after we've already gone through initializing graphics, so we end up making the wrong decision here:
#04: gfxPlatform::Init()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x173ee9a]
#05: gfxPlatform::GetPlatform()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x173e3da]
#06: mozilla::layers::CompositorChild::Create(IPC::Channel*, int)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x16bac4e]
#07: mozilla::dom::ContentChild::AllocPCompositorChild(IPC::Channel*, int)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x362933f]
#08: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xdf660c]
because it happens before this in ContentParent.cpp:
if (gAppData) {
nsCString version(gAppData->version);
...
// Sending all information to content process.
unused << SendAppInfo(version, buildID, name, UAName, ID, vendor);
}
Assignee | ||
Comment 1•10 years ago
|
||
Heads up - will probably want to track for 39/40, if the bug 1162530 it blocks ends up being tracked for those releases.
:smaug, Conley suggested you'd be the right person to tell us if it makes sense to initialize app data earlier.
Blocks: 1162530
Flags: needinfo?(bugs)
Assignee | ||
Comment 2•10 years ago
|
||
This fixes the problem for me, but I don't pretend to understand if app data is now initialized "too early".
Attachment #8602964 -
Flags: review?(bugs)
Assignee | ||
Comment 3•10 years ago
|
||
No need for needinfo as there's by now a review request.
Flags: needinfo?(bugs)
Comment 4•10 years ago
|
||
Comment on attachment 8602964 [details] [diff] [review]
SendAppInfo earlier, so that graphics has it. r=smaug
So, the comment says
"The CompositorChild must be created before any
PBrowsers are created, because they rely on the Compositor
already being around."
and SendAppInfo may actually create the child side of a PBrowser via PreloadSlowThings.
It is weird that SendAppInfo ends up doing any preloading stuff. What if
you just split that to two, and SendAppInfo could happen early, and then
the new Preload thing after aSetupOffMainThreadCompositing.
That would make the setup a tad easier to understand.
Attachment #8602964 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 5•10 years ago
|
||
I'll see if I can figure this out; also, I just realized we don't have to track for 39/40, as the blocking of bug 1162530 is only in the e10s case, where we're fine with the order of startup.
Assignee | ||
Comment 6•10 years ago
|
||
Is this the right direction for the split you had in mind? If so, any detailed pointers on the naming, other things to look out for, what needs more comments, etc., if not, any suggestions as to what would work better?
Attachment #8602964 -
Attachment is obsolete: true
Attachment #8604184 -
Flags: feedback?(bugs)
Comment 7•10 years ago
|
||
Comment on attachment 8604184 [details] [diff] [review]
WIP - split the SendAppInfo into two pieces.
Yup, this looks fine to me
Attachment #8604184 -
Flags: feedback?(bugs) → feedback+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee: nobody → milan
Updated•10 years ago
|
tracking-e10s:
--- → m7+
Assignee | ||
Comment 9•10 years ago
|
||
Same code as the WIP f+ patch, just changed the patch comment. Try run looks good.
Attachment #8604184 -
Attachment is obsolete: true
Attachment #8605853 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8605853 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Keywords: checkin-needed
Comment 11•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•