Bug 65323 – REGRESSION: r91931 causes NOTREACHED to be hit via StorageTracker

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adrienne Walker]

After http://trac.webkit.org/changeset/91931/, a number of Chromium tests are hitting a NOTREACHED in debug mode.

Stack trace from fast/js/global-constructors.html on Chromium Win Debug:

SHOULD NEVER BE REACHED
Backtrace:
	WebCore::SQLiteFileSystem::appendDatabaseFileNameToPath [0x0165E819+25] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\platform\sql\chromium\sqlitefilesystemchromium.cpp:68)
	WebCore::StorageTracker::trackerDatabasePath [0x0152D917+103] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\storage\storagetracker.cpp:100)
	WebCore::StorageTracker::openTrackerDatabase [0x0152DA01+193] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\storage\storagetracker.cpp:114)
	WebCore::StorageTracker::syncImportOriginIdentifiers [0x0152DD64+148] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\storage\storagetracker.cpp:159)
	WebCore::LocalStorageTask::performTask [0x0152AE05+117] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\storage\localstoragetask.cpp:94)
	WebCore::LocalStorageThread::threadEntryPoint [0x01528833+131] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\storage\localstoragethread.cpp:69)
	WebCore::LocalStorageThread::threadEntryPointCallback [0x0152879B+11] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\webcore\storage\localstoragethread.cpp:63)
	WTF::threadEntryPoint [0x020CA305+149] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\javascriptcore\wtf\threading.cpp:67)
	WTF::wtfThreadEntryPoint [0x008C18D9+89] (e:\b\build\slave\webkit_win__dbg__2_\build\src\third_party\webkit\source\javascriptcore\wtf\threadingwin.cpp:209)
	_callthreadstartex [0x00A7E783+83] (f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:348)
	_threadstartex [0x00A7E724+164] (f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:331)
	GetModuleFileNameA [0x7C80B729+442]

Full set of tests:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fjs%2Fglobal-constructors.html%2Cfast%2Floader%2Fchild-frame-add-after-back-forward.html%2Cfast%2Floader%2Fdocument-with-fragment-url-1.html%2Cfast%2Floader%2Fdocument-with-fragment-url-4.html%2Cfast%2Floader%2Fiframe-crash-on-missing-image.xhtml%2Cfast%2Floader%2Fjavascript-url-encoding-2.html%2Cfast%2Floader%2Fstateobjects%2Fdocument-destroyed-navigate-back-with-fragment-scroll.html%2Cfast%2Floader%2Fstateobjects%2Fpopstate-after-load-complete-body-attribute.html%2Cfast%2Floader%2Fstateobjects%2Fpushstate-clears-forward-history.html%2Cfast%2Floader%2Fstateobjects%2Freplacestate-updates-location.html%2Chttp%2Ftests%2Fappcache%2Flocal-content.html%2Chttp%2Ftests%2Fhistory%2Fback-to-post.php%2Chttp%2Ftests%2Finspector%2Fchange-iframe-src.html%2Chttp%2Ftests%2Finspector%2Fconsole-cd.html%2Chttp%2Ftests%2Finspector%2Fconsole-resource-errors.html%2Chttp%2Ftests%2Finspector%2Fconsole-websocket-error.html%2Chttp%2Ftests%2Finspector%2Fconsole-xhr-logging.html%2Chttp%2Ftests%2Finspector%2Finspect-iframe-from-different-domain.html%2Chttp%2Ftests%2Finspector%2Fnetwork-preflight-options.html%2Chttp%2Ftests%2Finspector%2Fresource-har-conversion.html

[Adrienne Walker]

This doesn't appear to happen in Safari, even in Debug builds.

[Brady Eidson]

Oh dear...

It's not at all clear to me from this backtrace exactly what's going on - why 91931 caused this.  I was under the impression that Chromium didn't make use of the StorageTracker (I remember when we were adding it there was a lot of hubbub about making sure it doesn't affect Chromium because they didn't want it)

91931 simple deferred StorageTracker initialization until the first time a storage area is accessed by the DOM.  That code path is common and always has to happen on the main thread.

From the background storage thread perspective, it is bizarre that there's any visible behavior change.

[Brady Eidson]

(In reply to comment #2)
> Oh dear...
> 
> It's not at all clear to me from this backtrace exactly what's going on - why 91931 caused this.  I was under the impression that Chromium didn't make use of the StorageTracker (I remember when we were adding it there was a lot of hubbub about making sure it doesn't affect Chromium because they didn't want it)
> 
> 91931 simple deferred StorageTracker initialization until the first time a storage area is accessed by the DOM.  That code path is common and always has to happen on the main thread.
> 
> From the background storage thread perspective, it is bizarre that there's any visible behavior change.

Oh duh, I think I realized what's going on.  By moving the initialization from WebKit to WebCore without platform guards, now *everyone* is getting an active, initialized StorageTracker.

That was a pretty bad mistake on my part.  My apologies - I'm working on a fix now.

[Adrienne Walker]

(In reply to comment #3)
 
> That was a pretty bad mistake on my part.  My apologies - I'm working on a fix now.

Thanks for taking a look, even though this looked ostensibly like a Chromium-only problem.  :)

[Brady Eidson]

Created attachment 102274 [details]
Patch v1 - Flip the initialization flag to have "needs initialization" meaning, and only set it when ::initializeTracker() has been called (which Chromium doesn't do)

[Brady Eidson]

Okay http://trac.webkit.org/changeset/91943 should make this go away.  Please close once you've seen things clear up on your tests.

[Adrienne Walker]

(In reply to comment #6)
> Okay http://trac.webkit.org/changeset/91943 should make this go away.  Please close once you've seen things clear up on your tests.

Slow debug builders are slow.  Finally, all green.  Thanks again.  :)