New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 608114 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocking:
issue 432959



Sign in to add a comment

Need to break dependency browser->blink

Project Member Reported by halliwell@chromium.org, Apr 30 2016

Issue description

Repro:
gn gen out/Default
ninja -C out/Default chrome

Build error:
LINK ./nacl_helper
FAILED: nacl_helper 
../../content/common/child_process_sandbox_support_impl_linux.cc:69: error: undefined reference to 'blink::WebFontRenderStyle::setDefaults()'
 
Cc: dpranke@chromium.org spang@chromium.org
Forgot to say, that was OS=linux.

chromium-dev thread from today mentions the error also happens when building 'chromeos':
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Mdjp5CMQVFQ

Fix for a similar-sounding issue from a few months back:
https://codereview.chromium.org/1682843002/

Blocking: 432959
Cc: mcgrathr@chromium.org bradnelson@chromium.org
Components: Build Platform>NaCl
Labels: Proj-GN-Migration OS-Linux
Status: Available (was: Untriaged)
Oh and this change from yesterday touched the same line as that last fix:
https://codereview.chromium.org/1926443002

I'm betting it's that, will confirm :)
Owner: scottmg@chromium.org
Confirmed, reverting that change fixes the link error.

scottmg@, please take a look.
Status: Assigned (was: Available)
I won't have time to look at this right away. You can revert if this is a config someone uses.
Personally, I'm unblocked by switching to component build.  I don't know how many people use debug static.
It's probably OK to "fix" these by adding bizarre dependencies to the nacl components.  Ideally things would be refactored better all over the place so that you don't have any dependency trail from nacl_helper to e.g. any part of blink, because that is just insane.  In practice it has so far proven insurmountable to worry about that from the nacl_helper perspective (not that the required refactorings wouldn't be their own inherent goods for general modularity and so forth) because the status quo is just such a tangled web.  This avoids being disastrous only because -ffunction-sections+--gc-sections clears out all the false dependencies quite well (and equivalent MSVC linker magic for the analogous nacl64.exe build).  But someone needs to stay wary of nacl_helper (and nacl64.exe) binary size jumps and ensure that any "fix" for a problem like this one doesn't actually add anything to these binary sizes (in the release build anyway).

That's just a drive-by because I'm out of the loop on NaCl maintenance by next week (short of dire needs, of course).
Brad will have to be the one (to delegate) worrying about making sure this stuff doesn't slide now.
Cc: nick@chromium.org
I'm "surprised and delighted" to hear that nacl_helper depends on Blink.

It does indeed look the same as https://codereview.chromium.org/1682843002/ where Nick commented it looked like the wrong fix. :/

It looks to me like exported/linux/WebFontRenderStyle.cpp should be in blink_minimal, and that should be the dependency, but I'm not too sure. My little Linux laptop is chugging along, but it might be a while.
Maybe https://codereview.chromium.org/1945473002 is right, I'm not sure.
Cc: blimp-bugs+garchive@google.com fsam...@chromium.org markdittmer@chromium.org w...@chromium.org scottmg@chromium.org
 Issue 608715  has been merged into this issue.
 Issue 608718  has been merged into this issue.
Alright, I give, reverting https://codereview.chromium.org/1926443002. We'll have to make our code not crazy another day.
Project Member

Comment 14 by bugdroid1@chromium.org, May 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a6abb71c16674052eec28cff8d16d15fda4577b

commit 8a6abb71c16674052eec28cff8d16d15fda4577b
Author: scottmg <scottmg@chromium.org>
Date: Tue May 03 18:50:16 2016

Revert of Turn on assert_no_deps for blink in browser (patchset #2 id:20001 of https://codereview.chromium.org/1926443002/ )

Reason for revert:
debug static linux is sadness.

BUG= 608114  and others.

Original issue's description:
> Turn on assert_no_deps for blink in browser
>
> R=brettw@chromium.org
> BUG= 582206 
>
> Committed: https://crrev.com/ffb95be04fb4effb750884083cb1c166c5499b7a
> Cr-Commit-Position: refs/heads/master@{#390123}

TBR=brettw@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 582206 

Review-Url: https://codereview.chromium.org/1947643002
Cr-Commit-Position: refs/heads/master@{#391314}

[modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/chrome/BUILD.gn
[modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/components/password_manager/content/public/cpp/BUILD.gn
[modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/content/browser/BUILD.gn
[modify] https://crrev.com/8a6abb71c16674052eec28cff8d16d15fda4577b/content/common/BUILD.gn

Cc: khushals...@chromium.org
Summary: Need to break dependency browser->blink (was: Link error in debug static builds )
I trial-and-errored this today for a while: https://codereview.chromium.org/1945473002/. It passes the bots (but hey, that's what they said last time).

Not sure if the Blink changes are kosher either.

blimp_shell->blink was added fairly recently in https://chromium.googlesource.com/chromium/src/+/12a558c16f853751ab702c84fbd0ffd7de565cdc. I gave blimp_shell a direct dependency on blink for that, because... I dunno. I'm not sure what that should be doing, maybe someone else has a thought.
Project Member

Comment 16 by bugdroid1@chromium.org, May 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/581bbb4d26224e4dce31e93651af0df157901c60

commit 581bbb4d26224e4dce31e93651af0df157901c60
Author: scottmg <scottmg@chromium.org>
Date: Fri May 06 18:35:16 2016

Break dependencies of browser -> blink

In order to get assert_no_deps on, move a couple small implementation files to
blink_common (WebInputEvent.cpp and WebFontRenderStyle.cpp).

blimp_shell adds a dependency on blink, which it needs for
blink::WebFontRendering::setSkiaFontManager. I'm not sure what to do about that
one.

BUG= 582206 , 608114 

Review-Url: https://codereview.chromium.org/1945473002
Cr-Commit-Position: refs/heads/master@{#392105}

[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/blimp/client/BUILD.gn
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/chrome/BUILD.gn
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/components/password_manager/content/public/cpp/BUILD.gn
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/content/browser/BUILD.gn
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/content/common/BUILD.gn
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/platform/BUILD.gn
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/platform/blink_platform.gyp
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/platform/blink_platform.gypi
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/web/WebInputEvent.cpp
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/Source/web/web.gypi
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/public/platform/linux/WebFontRenderStyle.h
[modify] https://crrev.com/581bbb4d26224e4dce31e93651af0df157901c60/third_party/WebKit/public/web/WebInputEvent.h

fixed?
Status: Fixed (was: Assigned)
Yes, I think this was fixed.
Blimp is still broken.
(or was.. I'm not sure if it got fixed. I guess it could be a new bug by now anyway.)
khushalsagar, I think you were looking at that, did you manage to figure out a way to avoid the dependency?
Cc: -scottmg@chromium.org mlliu@chromium.org
Sorry about the late reply, that dependency exists in browser on linux because we need to create the SkFontMgr for overriding blink's default font manager before the sandbox is initialized in the zygote process, because the font manager opens and cached fds on initialization. This is because we have to use the android font manager on the engine for an android client, and it assumes access to font files within the renderer sandbox.

I think the better solution would be to fix the font management setup here so we can rely on the browser for access to these files instead. This will either way be necessary for Blimp, crbug.com/601129 is tracking it.

Sign in to add a comment