Closed Bug 1403690 Opened 7 years ago Closed 7 years ago

stylo: Stop whitelisting LookAndFeel::GetColor in the heap write hazard analysis

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: bholley, Assigned: bradwerth)

References

Details

Attachments

(6 files)

This is a followup issue from bug 1354966 comment 25.

Right now LookAndFeel::GetColor is whitelisted in the heap-write analysis [1]. We have a lock, but that lock lives in Gecko_GetLookAndFeelSystemColor, which means that it wouldn't protect other call paths to LookAndFeel::GetColor, if they exist.

So the first-order fix here would be to whitelist Gecko_GetLookAndFeelSystemColor instead, alongside the other locked stuff.

However, things are extra tricky because bug 1401063 removed the platform API calls from LookAndFeel::GetColor in the gtk case, which means that the analysis (which runs on linux) would actually be if we didn't whitelist Gecko_GetLookAndFeelSystemColor at all. This is kind of a dangerous place to be, so we should probably strip the platform API calls from all the NativeGetColor implementations, which in turn would allow us to eliminate sServoWidgetLock entirely.

[1] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/js/src/devtools/rootAnalysis/analyzeHeapWrites.js#472
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #0)
> which means that the
> analysis (which runs on linux) would actually be if we didn't whitelist
> Gecko_GetLookAndFeelSystemColor at all.

s/actually be/actually be green/.
Assignee: nobody → bwerth
Blocks: 1403699
Priority: -- → P2
See Also: → 1354966
The current patches make progress on this, but are visually problematic at least on macOS (where I can easily test). They display incorrect colors in Chrome elements on macOS. The patches attempt to cache colors in nsLookAndFeel::NativeInit(), which seems to be intended for this purpose. But I see that ServoStyleSet::PreTraverseSync calls NativeInit on every traversal: http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/layout/style/ServoStyleSet.cpp#420.

Bobby, is NativeInit intended to be a one-time only init? Or is it appropriate to be called per traversal (perhaps to handle changes of OS theming while the app is open)?
Flags: needinfo?(bobbyholley)
(In reply to Brad Werth [:bradwerth] from comment #5)
> Bobby, is NativeInit intended to be a one-time only init? Or is it
> appropriate to be called per traversal (perhaps to handle changes of OS
> theming while the app is open)?

Bug 1386915 added NativeInit as a per-traversal call. At the time, only gtk/Linux did anything with NativeInit. This bug may change that and require change to the approach of calling it once per Stylo traversal.
Flags: needinfo?(bobbyholley)
See Also: → 1386915
(In reply to Brad Werth [:bradwerth] from comment #6)
> Bug 1386915 added NativeInit as a per-traversal call. At the time, only
> gtk/Linux did anything with NativeInit. This bug may change that and require
> change to the approach of calling it once per Stylo traversal.

I'm still not entirely sure why this matters. It's called every traversal, but it checks mInitialized and early-returns, so it's just a lazy init routine. I don't know why that's relevant to this bug.
And summarizing from the meeting:
* We should figure out if it's correct to cache the system colors on all the platforms. If so, we should do it on mac/windows. If not, we should back out the caching on linux, so that our linux-only static analysis properly recognizes that those codepaths trigger widget calls.
* We should probably go ahead and do bug 1403699 immediately, and just lock in Gecko_GetLookAndFeelSystemColor for now.
Attachment #8914566 - Flags: review?(jmathies)
Attachment #8914567 - Flags: review?(mstange)
Okay, got this working visually for macOS by doing the color caching as demanded by the first call to NativeGetColor, which is the approach taken by the existing, presumably correct gtk implementation. I'm now asking for review by DOM peers for macOS and Windows to see if this approach is acceptable on those platforms while I move ahead on implementation on Android and iOS.
For part 2, where is the code that invalidates the cached values when we receive a WM_SETTINGCHANGE?
(In reply to Jim Mathies [:jimm] from comment #13)
> For part 2, where is the code that invalidates the cached values when we
> receive a WM_SETTINGCHANGE?

I've updated all parts of the patch (including part 2 for Windows) to reset the mInitialized variable within the nsLookAndFeel::RefreshImpl() function. That function is called in platform-specific circumstances. On Windows, it is called in response to the WM_SETTINGCHANGE event.
Comment on attachment 8914567 [details]
Bug 1403690 Part 3: macOS change nsLookAndFeel::NativeGetColor to use cached colors.

https://reviewboard.mozilla.org/r/185902/#review193342

Caching these colors is fine as long as they're invalidated by a call to NotifyThemeChanged, which does seem to be the case.

There are a few instances of end-of-line whitespace in the patch.
Attachment #8914567 - Flags: review?(mstange) → review+
Attachment #8914565 - Flags: review?(manishearth)
Attachment #8917155 - Flags: review?(karlt)
Attachment #8917132 - Flags: review?(snorp)
Attachment #8917156 - Flags: review?(ted)
Comment on attachment 8917155 [details]
Bug 1403690 Part 4: gtk rearrange header and implementation to keep init and refresh functions together.

https://reviewboard.mozilla.org/r/188168/#review193390
Attachment #8917155 - Flags: review?(karlt) → review+
Comment on attachment 8914565 [details]
Bug 1403690 Part 1: Adjust heap write analysis whitelist to include Gecko_GetLookAndFeelSystemColor.

https://reviewboard.mozilla.org/r/185898/#review193420

::: js/src/devtools/rootAnalysis/analyzeHeapWrites.js:483
(Diff revision 3)
>          // Unable to trace through dataflow, but straightforward if inspected.
>          "Gecko_NewNoneTransform",
>  
>          // Need main thread assertions or other fixes.
>          /EffectCompositor::GetServoAnimationRule/,
> -        /LookAndFeel::GetColor/,
> +        /Gecko_GetLookAndFeelSystemColorr/,

two rs?
Attachment #8914565 - Flags: review?(manishearth) → review+
Comment on attachment 8914566 [details]
Bug 1403690 Part 2: Windows change nsLookAndFeel::NativeGetColor to use cached colors.

https://reviewboard.mozilla.org/r/185900/#review193694
Attachment #8914566 - Flags: review?(jmathies) → review+
Comment on attachment 8917132 [details]
Bug 1403690 Part 5: Android change pref and color caching to match approach used in gtk, macOS, and Windows.

https://reviewboard.mozilla.org/r/188136/#review194870
Attachment #8917132 - Flags: review?(snorp) → review+
Patches 1-5 have r+. We're just waiting for ted to review patch 6.

I don't think we need to uplift this to Beta 57.
Comment on attachment 8917156 [details]
Bug 1403690 Part 6: UIKit change nsLookAndFeel::NativeGetColor to use cached colors.

https://reviewboard.mozilla.org/r/188170/#review197156
Attachment #8917156 - Flags: review?(ted) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e820fb50295
Part 1: Adjust heap write analysis whitelist to include Gecko_GetLookAndFeelSystemColor. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/69668003e827
Part 2: Windows change nsLookAndFeel::NativeGetColor to use cached colors. r=jimm
https://hg.mozilla.org/integration/autoland/rev/c8262b239d17
Part 3: macOS change nsLookAndFeel::NativeGetColor to use cached colors. r=mstange
https://hg.mozilla.org/integration/autoland/rev/349f51081127
Part 4: gtk rearrange header and implementation to keep init and refresh functions together. r=karlt
https://hg.mozilla.org/integration/autoland/rev/6a5e056b7fb9
Part 5: Android change pref and color caching to match approach used in gtk, macOS, and Windows. r=snorp
https://hg.mozilla.org/integration/autoland/rev/29e6612a4083
Part 6: UIKit change nsLookAndFeel::NativeGetColor to use cached colors. r=ted
Backed out for build bustage in widget/windows/nsLookAndFeel.cpp on Windows and Android:

https://hg.mozilla.org/integration/autoland/rev/e242af21e3d2ec780a7fa9f56fbd2ce2e0c14d89
https://hg.mozilla.org/integration/autoland/rev/3003177f00f3aa9b359304fcc67d8acec08281fd
https://hg.mozilla.org/integration/autoland/rev/087a3875d2855c6f8d29a402f5f8a8aef431460f
https://hg.mozilla.org/integration/autoland/rev/9c19645d8078c70eb7d054ecf30fdfa00adfd186
https://hg.mozilla.org/integration/autoland/rev/888af9141ff82c22805a59e1e85425528ab20565
https://hg.mozilla.org/integration/autoland/rev/6ca4d18dede7c3536417e7c01e5fd6acfe4e2a3e

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=29e6612a4083dce3e7838e95f887bf77c61d2c2a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Build log Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=138924182&repo=autoland
z:/build/build/src/widget/windows/nsLookAndFeel.cpp(87): error C2723: 'nsLookAndFeel::NativeInit': 'final' specifier illegal on function definition
z:/build/build/src/widget/windows/nsLookAndFeel.cpp(294): error C2065: 'mHasAccentColor': undeclared identifier
z:/build/build/src/widget/windows/nsLookAndFeel.cpp(295): error C2065: 'mAccentColor': undeclared identifier
z:/build/build/src/widget/windows/nsLookAndFeel.cpp(302): error C2065: 'mHasAccentColorText': undeclared identifier
z:/build/build/src/widget/windows/nsLookAndFeel.cpp(303): error C2065: 'mAccentColorText': undeclared identifier
z:/build/build/src/widget/windows/nsLookAndFeel.cpp(886): error C2065: 'mHasAccentColor': undeclared identifier
z:/build/build/src/widget/windows/nsLookAndFeel.cpp(889): error C2065: 'mHasAccentColorText': undeclared identifier

Build log Android: https://treeherder.mozilla.org/logviewer.html#?job_id=138924174&repo=autoland
[task 2017-10-23T15:55:07.909Z] 15:55:07     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/widget/android/Unified_cpp_widget_android1.cpp:20:0:
[task 2017-10-23T15:55:07.910Z] 15:55:07     INFO -  /builds/worker/workspace/build/src/widget/android/nsLookAndFeel.cpp: In member function 'void nsLookAndFeel::EnsureInit()':
[task 2017-10-23T15:55:07.910Z] 15:55:07     INFO -  /builds/worker/workspace/build/src/widget/android/nsLookAndFeel.cpp:508:49: error: 'NS_SUCCESS' was not declared in this scope
[task 2017-10-23T15:55:07.910Z] 15:55:07     INFO -           mInitializedSystemColors = NS_SUCCESS(rv);
[task 2017-10-23T15:55:07.911Z] 15:55:07     INFO -                                                   ^
[task 2017-10-23T15:55:07.911Z] 15:55:07     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1072: recipe for target 'Unified_cpp_widget_android1.o' failed
[task 2017-10-23T15:55:07.911Z] 15:55:07     INFO -  make[5]: *** [Unified_cpp_widget_android1.o] Error 1
Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/34accef71609
Part 1: Adjust heap write analysis whitelist to include Gecko_GetLookAndFeelSystemColor. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/251b52554fee
Part 2: Windows change nsLookAndFeel::NativeGetColor to use cached colors. r=jimm
https://hg.mozilla.org/integration/autoland/rev/9861b50e325b
Part 3: macOS change nsLookAndFeel::NativeGetColor to use cached colors. r=mstange
https://hg.mozilla.org/integration/autoland/rev/7636f668f84e
Part 4: gtk rearrange header and implementation to keep init and refresh functions together. r=karlt
https://hg.mozilla.org/integration/autoland/rev/cc5bcbafc6cc
Part 5: Android change pref and color caching to match approach used in gtk, macOS, and Windows. r=snorp
https://hg.mozilla.org/integration/autoland/rev/04439a161b82
Part 6: UIKit change nsLookAndFeel::NativeGetColor to use cached colors. r=ted
Backed out for crashing many Android XPCshell runs:

https://hg.mozilla.org/integration/autoland/rev/0617fbf40ce4751e4f9a5565ea7e808c60eb2aba
https://hg.mozilla.org/integration/autoland/rev/3e8355db9d8867168ae99b8b9334e0dcf416cc39
https://hg.mozilla.org/integration/autoland/rev/925684bd136dfa101a3a9ef28f07a81b94d0c021
https://hg.mozilla.org/integration/autoland/rev/d341020e5fb5c94add8b641ef9007397877427db
https://hg.mozilla.org/integration/autoland/rev/3d856c36245d71a086e43496c88eacc1b3c98bb6
https://hg.mozilla.org/integration/autoland/rev/e945c00c958af4ce52235f886d1435b6f213bb7a

Range with push and failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=43e45144dd9fffa826b7b4d8289f77ddeea172c6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=android%20xpcshell&tochange=0617fbf40ce4751e4f9a5565ea7e808c60eb2aba&selectedJob=138955242
Unfortunately xpcshell tests are missing symbols (bug 1389805).
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=138955867&repo=autoland

[task 2017-10-23T18:24:58.072Z] 18:24:58     INFO -  TEST-START | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js
[task 2017-10-23T18:25:20.130Z] 18:25:20  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | xpcshell return code: 139
[task 2017-10-23T18:25:20.132Z] 18:25:20     INFO -  TEST-INFO took 22058ms
[task 2017-10-23T18:25:20.134Z] 18:25:20     INFO -  >>>>>>>
[task 2017-10-23T18:25:20.134Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | xpcw: cd /storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell
[task 2017-10-23T18:25:20.135Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head.js", "/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head_telemetry.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_ext_runtime_connect_no_receiver.js"]; -e const _TEST_NAME = "xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js" -e _execute_test(); quit(0);
[task 2017-10-23T18:25:20.135Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | [861, Unnamed thread 45c16080] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /builds/worker/workspace/build/src/xpcom/base/nsTraceRefcnt.cpp, line 171
[task 2017-10-23T18:25:20.136Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | [861, Main Thread] WARNING: No CID found when attempting to map contract ID: file /builds/worker/workspace/build/src/xpcom/components/nsComponentManager.cpp, line 508
[task 2017-10-23T18:25:20.136Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | [861, Main Thread] WARNING: Couldn't get the user appdata directory. Crash events may not be produced.: file /builds/worker/workspace/build/src/toolkit/crashreporter/nsExceptionHandler.cpp, line 2872
[task 2017-10-23T18:25:20.136Z] 18:25:20     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-10-23T18:25:20.136Z] 18:25:20     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2017-10-23T18:25:20.137Z] 18:25:20     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-10-23T18:25:20.137Z] 18:25:20     INFO -  running event loop
[task 2017-10-23T18:25:20.138Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | Starting check_remote
[task 2017-10-23T18:25:20.138Z] 18:25:20     INFO -  (xpcshell/head.js) | test check_remote pending (2)
[task 2017-10-23T18:25:20.138Z] 18:25:20     INFO -  TEST-PASS | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | check_remote - [check_remote : 241] useRemoteWebExtensions matches - false == false
[task 2017-10-23T18:25:20.139Z] 18:25:20     INFO -  TEST-PASS | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | check_remote - [check_remote : 241] testing from extension process - true == true
[task 2017-10-23T18:25:20.139Z] 18:25:20     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2017-10-23T18:25:20.139Z] 18:25:20     INFO -  (xpcshell/head.js) | test run_next_test 1 pending (2)
[task 2017-10-23T18:25:20.139Z] 18:25:20     INFO -  (xpcshell/head.js) | test check_remote finished (2)
[task 2017-10-23T18:25:20.140Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | Starting test_connect_without_listener
[task 2017-10-23T18:25:20.140Z] 18:25:20     INFO -  (xpcshell/head.js) | test test_connect_without_listener pending (2)
[task 2017-10-23T18:25:20.140Z] 18:25:20     INFO -  "Extension attached"
[task 2017-10-23T18:25:20.140Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | [861, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /builds/worker/workspace/build/src/netwerk/protocol/res/SubstitutingProtocolHandler.cpp, line 333
[task 2017-10-23T18:25:20.140Z] 18:25:20     INFO -  (xpcshell/head.js) | test run_next_test 1 finished (2)
[task 2017-10-23T18:25:20.141Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | [861, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file /builds/worker/workspace/build/src/toolkit/components/resistfingerprinting/nsRFPService.cpp, line 182
[task 2017-10-23T18:25:20.141Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | [861, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file /builds/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp, line 2874
[task 2017-10-23T18:25:20.142Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | [861, Main Thread] WARNING: Vsync not supported. Falling back to software vsync: file /builds/worker/workspace/build/src/gfx/thebes/gfxAndroidPlatform.cpp, line 401
[task 2017-10-23T18:25:20.142Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | Segmentation fault
[task 2017-10-23T18:25:20.143Z] 18:25:20     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | 13
[task 2017-10-23T18:25:20.143Z] 18:25:20     INFO -  <<<<<<<
[task 2017-10-23T18:25:20.410Z] 18:25:20     INFO -  mozcrash Copy/paste: /builds/worker/workspace/build/linux64-minidump_stackwalk /tmp/tmpqwMkKx/725db12b-7b39-2e8f-ec32-195c5d33c83d.dmp /builds/worker/workspace/build/symbols
[task 2017-10-23T18:25:20.779Z] 18:25:20     INFO -  mozcrash Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/725db12b-7b39-2e8f-ec32-195c5d33c83d.dmp
[task 2017-10-23T18:25:20.780Z] 18:25:20     INFO -  mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/725db12b-7b39-2e8f-ec32-195c5d33c83d.extra
[task 2017-10-23T18:25:20.782Z] 18:25:20  WARNING -  PROCESS-CRASH | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_runtime_connect_no_receiver.js | application crashed [@ libxul.so + 0x1398ea4]
[task 2017-10-23T18:25:20.782Z] 18:25:20     INFO -  Crash dump filename: /tmp/tmpqwMkKx/725db12b-7b39-2e8f-ec32-195c5d33c83d.dmp
[task 2017-10-23T18:25:20.782Z] 18:25:20     INFO -  Operating system: Android
[task 2017-10-23T18:25:20.782Z] 18:25:20     INFO -                    0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l
[task 2017-10-23T18:25:20.782Z] 18:25:20     INFO -  CPU: arm
[task 2017-10-23T18:25:20.782Z] 18:25:20     INFO -       ARMv7 ARM Cortex-A8 features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -       1 CPU
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -  GPU: UNKNOWN
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -  Crash reason:  SIGSEGV
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -  Crash address: 0x0
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -  Process uptime: not available
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -  Thread 0 (crashed)
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -   0  libxul.so + 0x1398ea4
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -       r0 = 0x00000078    r1 = 0x89e5edd7    r2 = 0x89e5edd7    r3 = 0x89e5edd7
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -       r4 = 0x00000000    r5 = 0x00000000    r6 = 0x00000000    r7 = 0x45c06854
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -       r8 = 0x00000000    r9 = 0xbebb4168   r10 = 0xbebb4248   r12 = 0x00000003
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -       fp = 0x00000000    sp = 0xbebb4010    lr = 0x41a3be13    pc = 0x41a3bea4
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -      Found by: given as instruction pointer in context
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -   1  libxul.so + 0x137a9ed
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -       sp = 0xbebb4028    pc = 0x41a1d9ef
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.786Z] 18:25:20     INFO -   2  libxul.so + 0x137ca05
[task 2017-10-23T18:25:20.787Z] 18:25:20     INFO -       sp = 0xbebb4030    pc = 0x41a1fa07
[task 2017-10-23T18:25:20.787Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.787Z] 18:25:20     INFO -   3  libxul.so + 0x1390a0f
[task 2017-10-23T18:25:20.788Z] 18:25:20     INFO -       sp = 0xbebb4058    pc = 0x41a33a11
[task 2017-10-23T18:25:20.788Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.788Z] 18:25:20     INFO -   4  libxul.so + 0x1390a59
[task 2017-10-23T18:25:20.789Z] 18:25:20     INFO -       sp = 0xbebb4060    pc = 0x41a33a5b
[task 2017-10-23T18:25:20.789Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.789Z] 18:25:20     INFO -   5  libxul.so + 0x1390a4f
[task 2017-10-23T18:25:20.790Z] 18:25:20     INFO -       sp = 0xbebb4064    pc = 0x41a33a51
[task 2017-10-23T18:25:20.790Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.790Z] 18:25:20     INFO -   6  libxul.so + 0x136e36d
[task 2017-10-23T18:25:20.791Z] 18:25:20     INFO -       sp = 0xbebb4070    pc = 0x41a1136f
[task 2017-10-23T18:25:20.791Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.791Z] 18:25:20     INFO -   7  libxul.so + 0x1cc8057
[task 2017-10-23T18:25:20.792Z] 18:25:20     INFO -       sp = 0xbebb4090    pc = 0x4236b059
[task 2017-10-23T18:25:20.792Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.793Z] 18:25:20     INFO -   8  libnss3.so + 0x94ad3
[task 2017-10-23T18:25:20.793Z] 18:25:20     INFO -       sp = 0xbebb4098    pc = 0x400c1ad5
[task 2017-10-23T18:25:20.793Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.794Z] 18:25:20     INFO -   9  libxul.so + 0x52a17
[task 2017-10-23T18:25:20.794Z] 18:25:20     INFO -       sp = 0xbebb40a0    pc = 0x406f5a19
[task 2017-10-23T18:25:20.794Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.795Z] 18:25:20     INFO -  10  libxul.so + 0x5574d
[task 2017-10-23T18:25:20.795Z] 18:25:20     INFO -       sp = 0xbebb40b0    pc = 0x406f874f
[task 2017-10-23T18:25:20.795Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.796Z] 18:25:20     INFO -  11  libxul.so + 0x1cc4f2f
[task 2017-10-23T18:25:20.796Z] 18:25:20     INFO -       sp = 0xbebb40d0    pc = 0x42367f31
[task 2017-10-23T18:25:20.796Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.797Z] 18:25:20     INFO -  12  libxul.so + 0x2bdc37e
[task 2017-10-23T18:25:20.797Z] 18:25:20     INFO -       sp = 0xbebb40f8    pc = 0x4327f380
[task 2017-10-23T18:25:20.797Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.798Z] 18:25:20     INFO -  13  libxul.so + 0x1c0c131
[task 2017-10-23T18:25:20.798Z] 18:25:20     INFO -       sp = 0xbebb4138    pc = 0x422af133
[task 2017-10-23T18:25:20.798Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.798Z] 18:25:20     INFO -  14  libxul.so + 0x561169
[task 2017-10-23T18:25:20.799Z] 18:25:20     INFO -       sp = 0xbebb4150    pc = 0x40c0416b
[task 2017-10-23T18:25:20.799Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.806Z] 18:25:20     INFO -  15  libxul.so + 0x1c0bf07
[task 2017-10-23T18:25:20.806Z] 18:25:20     INFO -       sp = 0xbebb418c    pc = 0x422aef09
[task 2017-10-23T18:25:20.806Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.806Z] 18:25:20     INFO -  16  libxul.so + 0x9e095
[task 2017-10-23T18:25:20.806Z] 18:25:20     INFO -       sp = 0xbebb41a8    pc = 0x40741097
[task 2017-10-23T18:25:20.806Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.806Z] 18:25:20     INFO -  17  libxul.so + 0x56d6c3
[task 2017-10-23T18:25:20.807Z] 18:25:20     INFO -       sp = 0xbebb41d8    pc = 0x40c106c5
[task 2017-10-23T18:25:20.807Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.807Z] 18:25:20     INFO -  18  libxul.so + 0x56d7b1
[task 2017-10-23T18:25:20.807Z] 18:25:20     INFO -       sp = 0xbebb4208    pc = 0x40c107b3
[task 2017-10-23T18:25:20.807Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.808Z] 18:25:20     INFO -  19  libxul.so + 0x5777cf
[task 2017-10-23T18:25:20.808Z] 18:25:20     INFO -       sp = 0xbebb4228    pc = 0x40c1a7d1
[task 2017-10-23T18:25:20.808Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.808Z] 18:25:20     INFO -  20  libnss3.so + 0x94ad3
[task 2017-10-23T18:25:20.808Z] 18:25:20     INFO -       sp = 0xbebb4240    pc = 0x400c1ad5
[task 2017-10-23T18:25:20.808Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.808Z] 18:25:20     INFO -  21  libnss3.so + 0x94ad3
[task 2017-10-23T18:25:20.809Z] 18:25:20     INFO -       sp = 0xbebb4250    pc = 0x400c1ad5
[task 2017-10-23T18:25:20.809Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.809Z] 18:25:20     INFO -  22  libnss3.so + 0x94ad3
[task 2017-10-23T18:25:20.809Z] 18:25:20     INFO -       sp = 0xbebb4260    pc = 0x400c1ad5
[task 2017-10-23T18:25:20.809Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.809Z] 18:25:20     INFO -  23  libxul.so + 0x52a17
[task 2017-10-23T18:25:20.809Z] 18:25:20     INFO -       sp = 0xbebb4268    pc = 0x406f5a19
[task 2017-10-23T18:25:20.810Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.810Z] 18:25:20     INFO -  24  libxul.so + 0x2b32ca4
[task 2017-10-23T18:25:20.810Z] 18:25:20     INFO -       sp = 0xbebb4274    pc = 0x431d5ca6
[task 2017-10-23T18:25:20.810Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.810Z] 18:25:20     INFO -  25  libxul.so + 0x5574d
[task 2017-10-23T18:25:20.810Z] 18:25:20     INFO -       sp = 0xbebb4278    pc = 0x406f874f
[task 2017-10-23T18:25:20.811Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.811Z] 18:25:20     INFO -  26  libxul.so + 0x217ab29
[task 2017-10-23T18:25:20.811Z] 18:25:20     INFO -       sp = 0xbebb42a0    pc = 0x4281db2b
[task 2017-10-23T18:25:20.811Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.811Z] 18:25:20     INFO -  27  libxul.so + 0x3c215
[task 2017-10-23T18:25:20.811Z] 18:25:20     INFO -       sp = 0xbebb42b0    pc = 0x406df217
[task 2017-10-23T18:25:20.812Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.812Z] 18:25:20     INFO -  28  libxul.so + 0x54346d
[task 2017-10-23T18:25:20.812Z] 18:25:20     INFO -       sp = 0xbebb42c8    pc = 0x40be646f
[task 2017-10-23T18:25:20.812Z] 18:25:20     INFO -      Found by: stack scanning
[task 2017-10-23T18:25:20.812Z] 18:25:20     INFO -  29  libxul.so + 0x572c81
[task 2017-10-23T18:25:20.812Z] 18:25:20     INFO -       sp = 0xbebb42e0    pc = 0x40c15c83
[task 2017-10-23T18:25:20.813Z] 18:25:20     INFO -      Found by: stack scanning
I figured it out; the Android implementation was fetching both SystemColors and ShowPassword during the same call, which was problematic. I split the initialization method into two parts, and only init each part when needed, and the tests work again.
Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b8017ae9cc1
Part 1: Adjust heap write analysis whitelist to include Gecko_GetLookAndFeelSystemColor. r=manishearth
https://hg.mozilla.org/integration/autoland/rev/cf0f2ec461a7
Part 2: Windows change nsLookAndFeel::NativeGetColor to use cached colors. r=jimm
https://hg.mozilla.org/integration/autoland/rev/1570e72037e2
Part 3: macOS change nsLookAndFeel::NativeGetColor to use cached colors. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4df8244690cf
Part 4: gtk rearrange header and implementation to keep init and refresh functions together. r=karlt
https://hg.mozilla.org/integration/autoland/rev/436c11e3c389
Part 5: Android change pref and color caching to match approach used in gtk, macOS, and Windows. r=snorp
https://hg.mozilla.org/integration/autoland/rev/c9589d98c0b8
Part 6: UIKit change nsLookAndFeel::NativeGetColor to use cached colors. r=ted
> Part 1: Adjust heap write analysis whitelist to include Gecko_GetLookAndFeelSystemColor

Why are we doing this? Isn't the whole point of this patch set to make Gecko_GetLookAndFeelSystemColor not call into any external code? It would seem that whitelisting in this way prevents the static analysis from verifying that this was all done correctly.
Flags: needinfo?(manishearth)
Flags: needinfo?(bwerth)
Blocks: 1412868
(In reply to Bobby Holley (parental leave - send mail for anything urgent) from comment #66)
> > Part 1: Adjust heap write analysis whitelist to include Gecko_GetLookAndFeelSystemColor
> 
> Why are we doing this? Isn't the whole point of this patch set to make
> Gecko_GetLookAndFeelSystemColor not call into any external code? It would
> seem that whitelisting in this way prevents the static analysis from
> verifying that this was all done correctly.

You're right; I wasn't thinking clearly. I filed Bug 1412868 to sort this out and I'll have a patch shortly.
Flags: needinfo?(manishearth)
Flags: needinfo?(bwerth)
Depends on: 1417356
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: