Issue 128422 in chromium: Browser crash in InstantFieldTrial::GetMode

[chro...@googlecode.com]

Status: Untriaged
Owner: ----
Labels: Type-Bug Pri-2 Area-Internals Feature-Instant Stability-Crash

New issue 128422 by the...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

Version: 20.0.1132.3
OS: Windows, Linux

http://crash/reportdetail?reportid=499eac0537b05b5a

Thread 0 *CRASHED* ( SIGSEGV @ 0x00000000 )

0x7fd0b194d1a0 [chrome] -
chrome/browser/instant/instant_field_trial.cc:77] InstantFieldTrial::GetMode
0x7fd0b194b2a8 [chrome] -
chrome/browser/instant/instant_controller.cc:80]
InstantController::IsEnabled
0x7fd0b16c3444 [chrome] - chrome/browser/ui/browser.cc:4230]
Browser::Observe
0x7fd0b19c39d3 [chrome] - chrome/browser/prefs/pref_notifier_impl.cc:108]
PrefNotifierImpl::FireObservers
0x7fd0b164fa17 [chrome] - chrome/browser/prefs/pref_value_store.cc:130]
PrefValueStore::NotifyPrefChanged
0x7fd0b19aaf72 [chrome] -
chrome/browser/policy/configuration_policy_pref_store.cc:123]
policy::ConfigurationPolicyPrefStore::Refresh
...

http://crash/reportdetail?reportid=574f0b592ccc7770
Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x0000008e )

0x022fc553 [chrome.dll] - instant_field_trial.cc:77]
InstantFieldTrial::GetMode(Profile *)
0x02f13139 [chrome.dll] - instant_controller.cc:183]
InstantController::SetOmniboxBounds(gfx::Rect const &)
0x02fdd45d [chrome.dll] - autocomplete_edit.cc:943]
AutocompleteEditModel::OnResultChanged(bool)
0x0248dc5c [chrome.dll] - autocomplete.cc:1102]
AutocompleteController::NotifyChanged(bool)
0x0248c4ac [chrome.dll] - autocomplete.cc:1038]
AutocompleteController::UpdateResult(bool)
0x02ebb98d [chrome.dll] - autocomplete.cc:981]
AutocompleteController::ExpireCopiedEntries()
0x0211e7e8 [chrome.dll] - timer.cc:179] base::Timer::RunScheduledTask()

[chro...@googlecode.com]

Comment #5 on issue 128422 by sre...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

Thanks, Eric. I guess you looked at the second crash (the path through
InstantController::SetOmniboxBounds, which is what refers to |
tab_contents_->profile()|). If you would be so nice as to look at the first
crash as well, that would be very helpful. Because, in the first crash,
Instant is just passing along the profile pointer it receives from
browser.cc, so it will be nice to know if the dangling pointer issue is
specific to Instant or more general.

[chro...@googlecode.com]

Comment #6 on issue 128422 by ero...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

I looked at: http://crash/reportdetail?reportid=574f0b592ccc7770

I only have the ability to get extra information for Windows minidumps, so
I can't tell you anything conclusive about the first link.

However given that it is on the same line of instant_field_trial.cc:77, and
that line has a NULL check, I expect that is the same problem (use after
free of a profile object).

[chro...@googlecode.com]

Updates:
Labels: -Pri-2 Pri-1

Comment #7 on issue 128422 by the...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

On Linux 20.0.1132.17, this is the most common browser crash.

[chro...@googlecode.com]

Updates:
Cc: -sre...@chromium.org the...@chromium.org b...@chromium.org
s...@chromium.org j...@chromium.org o...@chromium.org

Comment #9 on issue 128422 by sre...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

[cc:ing a bunch of people who may have some advice; sorry for the wide net]

I haven't been able to figure out the *entire* mechanism of how this bug
triggers, but here's what I do know:

The stack trace in the Linux crash report (e.g.:
http://crash/reportdetail?reportid=11ba49e951e38caf#crashing_thread) makes
it very clear what's happening: When the X server goes away,
the "ShuttingDownWithoutClosingBrowsers" path is taken. Profile objects are
destroyed. Then, one of the destructors causes "prefs changed"
notifications to be sent, which is received by Browser. It tries to look up
a profile, and crashes.

What's not clear to me is:

1. Along the paths I identified, it seemed like the pref observers were
being unregistered before the "prefs changed" notifications were sent, so I
don't know under what circumstances we end up with a Browser observer still
in the registered list. (I.e., I haven't been able to reproduce the crash.)

2. This doesn't seem to be crashing in M21. What changed?

3. This seems to be happening predominantly on Linux, specifically 3.2.*
kernels. Why not as much on 2.6.*, and why not as much on Windows (which
also has a ShuttingDownWithoutClosingBrowsers path)?

The real question of course, is, what's a good way to fix this? Should I
check for browser_shutdown::IsTryingToQuit() when handling pref changed
notifications in browser.cc? Something more general?

Advice welcome and appreciated!

[chro...@googlecode.com]

Updates:
Cc: cr...@chromium.org

Comment #10 on issue 128422 by o...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

(No comment was entered for this change.)

[chro...@googlecode.com]

Comment #11 on issue 128422 by the...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

FWIW, in response to (3), actually 77% of the crashes are with kernel
2.6.x. I don't think that has anything to with the crash.

In response to (2) - Not sure, but I see this on the 1132 dev channel but
not the 1145 dev channel.

Have you tried reproducing it? Perhaps leave Chrome running and kill X? If
you can reproduce it, you may be able to bisect between 1132 and 1145 to
see what fixed it for dev channel.

[chro...@googlecode.com]

Updates:
Labels: Action-BisectNeeded ReleaseBlock-Stable OS-Linux

Comment #12 on issue 128422 by dhar...@google.com: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

marking it as a blocker as 50% of browser crashes were coming from this.

[chro...@googlecode.com]

Updates:
Labels: Mstone-20

Comment #13 on issue 128422 by anan...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

[chro...@googlecode.com]

Comment #14 on issue 128422 by j...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

Perhaps the first thing to ask is whether PolicyServiceImpl should be
expected to have a dependency on Profile at shutdown time. If yes, then it
would seem right to move the call to browser_policy_connector_.reset() in
BrowserProcessImpl::StartTearDown up a few lines, above where
profile_manager_ is reset. If no, then neutering that dependency before
profiles are destroyed or doing something to detect that we're in the
middle of shutdown and not notifying observers of PolicyServiceImpl if this
is the case might be right.

I'm not sure what might have changed to make this crash go away in M21.

[chro...@googlecode.com]

Comment #15 on issue 128422 by dhar...@google.com: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

Here are the changes that went between 20.0.1130.0 and 21.0.1145.0

http://build.chromium.org/f/chromium/perf/dashboard/ui/changelog.html?url=/trunk/src&range=135804:138079&mode=html

[chro...@googlecode.com]

Comment #17 on issue 128422 by j...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

It sounds reasonable to try the simple fix for M20, i.e. check
IsTryingToQuit.

On trunk, I would suggest at least sending a change to the trybots where
you move destruction of PolicyServiceImpl up above where ProfileManager is
reset, to see whether that works. If it does, it should fix the current
bug but there is a chance it might expose something new and similarly
subtle/non-deterministic (our shutdown order is very complex) which is why
perhaps that should only go to trunk for now.

[chro...@googlecode.com]

Updates:
Labels: Merge-Requested

Comment #18 on issue 128422 by sre...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

Merge requested for r141554.

[chro...@googlecode.com]

Updates:
Labels: -Merge-Requested Merge-Approved

Comment #19 on issue 128422 by dhar...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

[chro...@googlecode.com]

Comment #20 on issue 128422 by bugdro...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422#c20

The following revision refers to this bug:
http://src.chromium.org/viewvc/chrome?view=rev&revision=141554

------------------------------------------------------------------------
r141554 | sre...@chromium.org | Mon Jun 11 16:27:10 PDT 2012

Changed paths:
M
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/browser.cc?r1=141554&r2=141553&pathrev=141554

Avoid crash in trying to access profile while shutting down.

See http://code.google.com/p/chromium/issues/detail?id=128422#c9 for
details.

BUG=128422
TEST=Watch the crash rate. I don't have a reproducible test case.

Review URL: https://chromiumcodereview.appspot.com/10536106
------------------------------------------------------------------------

[chro...@googlecode.com]

Updates:
Labels: merge-merged-1132

Comment #21 on issue 128422 by bugdro...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422#c21

The following revision refers to this bug:

http://src.chromium.org/viewvc/chrome?view=rev&revision=141561

------------------------------------------------------------------------
r141561 | sre...@chromium.org | Mon Jun 11 16:38:59 PDT 2012

Changed paths:
M
http://src.chromium.org/viewvc/chrome/branches/1132/src/chrome/browser/ui/browser.cc?r1=141561&r2=141560&pathrev=141561

Merge 141554 - Avoid crash in trying to access profile while shutting down.

See http://code.google.com/p/chromium/issues/detail?id=128422#c9 for
details.

BUG=128422
TEST=Watch the crash rate. I don't have a reproducible test case.

Review URL: https://chromiumcodereview.appspot.com/10536106

TBR=sre...@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10538095
------------------------------------------------------------------------

[chro...@googlecode.com]

Updates:
Labels: -Pri-1 -Action-BisectNeeded -ReleaseBlock-Stable Pri-2

Comment #22 on issue 128422 by sre...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

The crash should be fixed, but keeping it around to work on the root cause.

[chro...@googlecode.com]

Comment #23 on issue 128422 by sre...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422

Just a quick update that indeed, the latest beta (20.0.1132.34) doesn't see
this crash anymore.

[chro...@googlecode.com]

Updates:
Status: Fixed

Comment #25 on issue 128422 by sre...@chromium.org: Browser crash in
InstantFieldTrial::GetMode
http://code.google.com/p/chromium/issues/detail?id=128422