Should check whether document.settings() is available or not when updating GenericFontFamilySetting…

Should check whether document.settings() is available or not when updating GenericFontFamilySettings.

Looking at crash reports - WebCore::StyleEngine::updateGenericFontFamilySettings, document().settings() is probably null. In the case, we should not update GenericFontSettings.

BUG=376525
TEST=It is very hard to reproduce crashes reported in crbug.com/376525. Will check # of crash reports to see whether the bug is really fixed or not.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175783

[tasak, 2014-05-29 10:28:15]

Would you review this CL?

And would you advise me to add test (unit test, I guess) for this issue?

Crash link is 
crash.corp.google.com/browse?q=product.name%3D'Chrome_Android'%20AND%20product.version%3D'35.0.1916.122'%20AND%20custom_data.ChromeCrashProto.ptype%3D'renderer'%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D'WebCore%3A%3AStyleEngine%3A%3AupdateGenericFontFamilySettings'%20AND%20ReportID%3D'a74cb216f75d1c1f'#3

[dglazkov, 2014-05-29 22:24:28]

https://codereview.chromium.org/304033005/diff/30001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/304033005/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:534: if (!document().isActive() ||
!document().settings())
Just checking: is this a symptom of a larger problem? Is it ok for
WebSettingsImpl to call this on a frameless doc?

[tasak, 2014-05-30 02:12:05]

Thank you for reviewing.

https://codereview.chromium.org/304033005/diff/30001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/304033005/diff/30001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:534: if (!document().isActive() ||
!document().settings())
On 2014/05/29 22:24:28, dglazkov wrote:
> Just checking: is this a symptom of a larger problem? Is it ok for
> WebSettingsImpl to call this on a frameless doc?

As far as I investigated,

WebSettingsImpl::setStandardFontFamily (i.e. m_settings != null)
 ==> Settings::notifyGenericFontFamilyChange()
 ==> Page::settingsChanged()
    (obtain StyleEngine instance by using mainFrame() and frame->document() and
document()->styleEngine())
 ==> StyleEngine::updateGenericFontFamilySettings()

Looking at Document::settings(),
 - m_frame->settings() used in Document::settings
 - m_host->settings() used in Frame::settings()
 - m_page->settings() used in FrameHost::settings()
 - Page has OwnPtr<Settings> (strictly speaking, SettingsDelegate owns.)

I think,
  - Page has non-null m_settings, because Page creates Settings in its
constructor (i.e. SettingsDelegate(Settings::create()) )
  - FrameHost also has non-null m_page, because Page creates FrameHost in its
constructor. (i.e. m_frameHost(FrameHost::create(*this)) )
  - Document has non-null m_frame, because frame->document() seems to work.

So LocalFrame seems not to have m_host.

Talking about "it's ok or not", each settings() says "settings() can return
null". LocalFrame's constructor takes "FrameHost*" (not FrameHost&).

So this "no FrameHost localframe" would be expected (by someone who added the
comment).

[eseidel, 2014-05-30 02:14:44]

You will always have settings() while the document is active.  Active means it's
in a frame and being displayed at the time.

[eseidel, 2014-05-30 02:15:14]

Then again, that begs the question as to why a non-active document would ever
have a StyleEngine. :)

[esprehn, 2014-05-30 02:17:30]

On 2014/05/30 02:15:14, eseidel wrote:
> Then again, that begs the question as to why a non-active document would ever
> have a StyleEngine. :)

HTML Imports documents do that :/

[eseidel, 2014-05-30 02:18:36]

Why would an import need a style engine?

I think we need to explicitly document what isActive means in the
DocumentLifecycle, we shouldn't have this confusion. :)

[tasak, 2014-05-30 03:19:37]

On 2014/05/30 02:15:14, eseidel wrote:
> Then again, that begs the question as to why a non-active document would ever
> have a StyleEngine. :)

I chatted with haraken@.

He said, when document is detached but not destroyed, document still has style
engine.

However, if document is not active, probably we cannot invoke
StyleEngine::updateGenericFontSettings.

I will add "assert(document.isActive())" instead of the "if".

[dglazkov, 2014-05-30 15:08:06]

LGTM.

I just had a thought: since Document members' return values expectations change
through its lifecycle, we should have a thin wrapper around the Document that
reflects these expectations (with asserts and stuff). Then, instead of querying
Document directly, use that wrapper at callsites:

ActiveDocument(document).settings() <-- will assert if ain't got no settings.

WDYT?

[dglazkov, 2014-05-30 15:08:23]

TEST=WIP?

[dglazkov, 2014-05-30 15:21:42]

Wait... Does this change even make sense?

The only way you could get to the point where Document::settings() returns 0 is
when the Document has no m_frame.

At the same time, we get to StyleEngine::updateGenericFontFamilySettings by
traversing frames on the page.

So clearly, we're in the situation where the page thinks we have a frame with a
document, but the document thinks the opposite. That seems bad -- and not
something to wallpaper over.

[tasak, 2014-06-02 03:54:21]

On 2014/05/30 15:21:42, dglazkov wrote:
> Wait... Does this change even make sense?
> 
> The only way you could get to the point where Document::settings() returns 0
is
> when the Document has no m_frame.
> 
> At the same time, we get to StyleEngine::updateGenericFontFamilySettings by
> traversing frames on the page.
> 
> So clearly, we're in the situation where the page thinks we have a frame with
a
> document, but the document thinks the opposite. That seems bad -- and not
> something to wallpaper over.

Yeah, so I think, we really need how to reproduce this crash.

This patch is just for reducing crashes and does not fix any root cause.
(So this is the reason why still TEST=WIP)

[esprehn, 2014-06-04 18:54:28]

It's really suspect that the document could be in the frame tree but not active,
that doesn't seem right at all, especially not at the point where we're getting
a settings change notification from the embedder.

Please make that comment a FIXME to remove this check before landing.

lgtm

[tasak, 2014-06-05 12:05:03]

Thank you for reviewing.

I'm still trying to reproduce crashes and to create a test.

On 2014/06/04 18:54:28, esprehn wrote:
> It's really suspect that the document could be in the frame tree but not
active,
> that doesn't seem right at all, especially not at the point where we're
getting
> a settings change notification from the embedder.
> 
> Please make that comment a FIXME to remove this check before landing.

I would like to confirm what this check is? So ASSERT(document().isActive())?

I have already removed "if (!document().isActive())" from my patch and added the
ASSERT instead.

I think, ASSERT is ok for ensuring that only active document is in the frame
tree.

[tasak, 2014-06-06 06:30:23]

Can I land this patch?

I added FIXME comment to ASSERT(document.isActive()) and  updated the TEST line,
i.e. "I will check # of crash reports".

[dglazkov, 2014-06-06 14:39:16]

On 2014/06/06 06:30:23, tasak wrote:
> Can I land this patch?
> 
> I added FIXME comment to ASSERT(document.isActive()) and  updated the TEST
line,
> i.e. "I will check # of crash reports".

sure. You have to LGTMs on it :)

[abarth-chromium, 2014-06-06 17:02:02]

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:528: // FIXME: in-active documents should not be
in any frame tree.
That's not accurate.  Documents become inactive and then they leave the frame
tree.  During shutdown, there is a period of time after they become inactive
before they are removed from the frame tree.

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:530: // remove the fllowing ASSERT.
fllowing -> following

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:534: // document has settings or not.
crbug.com/376525
Please remove this comment. If settings() couldn't be null, then there would be
no need for line 535. Instead of a comment referring to a bug, we should have an
automated test that triggers the condition.

[abarth-chromium, 2014-06-06 17:05:29]

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:536: return;
Why are you checking the Settings object here? I don't see any references to the
Settings object in this function.  Typically we null-check the Settings object
immediately before accessing it.

[tasak, 2014-06-09 04:41:26]

Thank you for reviewing.

I updated the comment (and removed some comment).
However, I'm still trying to find how to reproduce reported crash. I have no
idea about how to trigger the condition: document.settings() == null.

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:528: // FIXME: in-active documents should not be
in any frame tree.
On 2014/06/06 17:02:02, abarth wrote:
> That's not accurate.  Documents become inactive and then they leave the frame
> tree.  During shutdown, there is a period of time after they become inactive
> before they are removed from the frame tree.

Thank you. I updated the comment.

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:530: // remove the fllowing ASSERT.
On 2014/06/06 17:02:02, abarth wrote:
> fllowing -> following

Done.

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:534: // document has settings or not.
crbug.com/376525
On 2014/06/06 17:02:02, abarth wrote:
> Please remove this comment. If settings() couldn't be null, then there would
be
> no need for line 535. Instead of a comment referring to a bug, we should have
an
> automated test that triggers the condition.

I agree that we should have automated test.

However, I cannot reproduce this issue. So I have no idea about how to trigger
the condition.

https://codereview.chromium.org/304033005/diff/70001/Source/core/dom/StyleEng...
Source/core/dom/StyleEngine.cpp:536: return;
On 2014/06/06 17:05:29, abarth wrote:
> Why are you checking the Settings object here? I don't see any references to
the
> Settings object in this function.  Typically we null-check the Settings object
> immediately before accessing it.

I see. I moved the check to CSSFontSelector::updateGenericFontFamilySettings.

[abarth-chromium, 2014-06-09 05:01:29]

LGTM

It's a shame not to have a test for this CL...

[tasak, 2014-06-09 09:07:10]

Thank you for reviewing.

[tasak, 2014-06-09 09:07:13]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-06-09 09:08:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tasak@google.com/304033005/110001

[commit-bot: I haz the power, 2014-06-09 09:16:12]

Change committed as 175783