Closed Bug 1162700 Opened 9 years ago Closed 9 years ago

With e10s, need to initialize app-info sooner in the content process - before graphics is


(Core :: DOM: Content Processes, defect)

40 Branch
Not set



Tracking Status
e10s m7+ ---
firefox40 --- affected
firefox41 --- fixed


(Reporter: milan, Assigned: milan)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

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/ +0x173ee9a]
#05: gfxPlatform::GetPlatform()[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/ +0x173e3da]
#06: mozilla::layers::CompositorChild::Create(IPC::Channel*, int)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/ +0x16bac4e]
#07: mozilla::dom::ContentChild::AllocPCompositorChild(IPC::Channel*, int)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/ +0x362933f]
#08: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&)[/Users/msreckovic/Repos/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/ +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);
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)
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)
No need for needinfo as there's by now a review request.
Flags: needinfo?(bugs)
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-
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.
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 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+
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)
Attachment #8605853 - Flags: review?(bugs) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.