1536575 - Stack size on aarch64 Windows should match stack size on x86_64 Windows | windows/aarch64 - REFTEST PROCESS-CRASH | build/tests/reftest/tests/layout/reftests/bugs/256180-1.html, 256180-2.html, 256180-3.html | application crashed

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P4)

Description

[Edwin Takahashi (:egao | infrequent contributor)]

+++ This bug was initially created as a clone of Bug #1530033 +++

The initial bug at 1530033 was used by :egao to disable the test from running for future pushes of windows10-aarch64. This bug is to implement the fix in the build system.

For discussions until cloning of the bug, please refer to the parent bug.

#[markdown(off)]
Filed by: egao [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=229891968&repo=try

https://queue.taskcluster.net/v1/task/EuOcrzLaSr66GujjsQKNQw/runs/0/artifacts/public/logs/live_backing.log

https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/EuOcrzLaSr66GujjsQKNQw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Raised during reftest-no-accel-e10s-2 on win10/aarch64.

Context:

14:47:20 INFO - REFTEST PROCESS-CRASH | file:///C:/tasks/task_1550846210/build/tests/reftest/tests/layout/reftests/bugs/256180-1.html | application crashed [@ static union core::option::Option<servo_arc::Arc<indexmap::map::IndexMap<style::gecko_string_cache::Atom, servo_arc::Arc<style::custom_properties::VariableValue>, core::hash::BuildHasherDefault<style::selector_map::PrecomputedHasher>>>> style::custom_properties::CustomPropertiesBuilder::build()]
14:47:20 INFO - Crash dump filename: c:\users\testdr~1\appdata\local\temp\tmp1u_n7p.mozrunner\minidumps\a7f7d801-397f-4fa6-93db-54a41cf13002.dmp
14:47:20 INFO - Operating system: Windows NT
14:47:20 INFO - 10.0.17134
14:47:20 INFO - CPU: 0x000c
14:47:20 INFO - 8 CPUs
14:47:20 INFO -
14:47:20 INFO - GPU: UNKNOWN
14:47:20 INFO -
14:47:20 INFO - Crash reason: EXCEPTION_STACK_OVERFLOW
14:47:20 INFO - Crash address: 0x7ffca0b59144
14:47:20 INFO - Assertion: Unknown assertion type 0x00000000
14:47:20 INFO - Process uptime: 6 seconds
14:47:20 INFO -
14:47:20 INFO - Thread 0 (crashed)
14:47:20 INFO - 0 xul.dll!static union core::option::Option<servo_arc::Arc<indexmap::map::IndexMap<style::gecko_string_cache::Atom, servo_arc::Arc<style::custom_properties::VariableValue>, core::hash::BuildHasherDefault<style::selector_map::PrecomputedHasher>>>> style::custom_properties::CustomPropertiesBuilder::build() [custom_properties.rs:e92ff56d2be21676b447c6fbb87b4c4479539bc9 : 610 + 0x10]
14:47:20 INFO -
14:47:20 INFO - Found by: given as instruction pointer in context
14:47:20 INFO - 1 xul.dll!static struct servo_arc::Arc<style::gecko_properties::ComputedValues> style::properties::cascade::cascade_rules<style::gecko::wrapper::GeckoElement>(struct style::gecko::media_queries::Device , union core::option::Option<style::gecko::pseudo_element::PseudoElement>, struct style::rule_tree::StrongRuleNode , struct style::shared_lock::StylesheetGuards , union core::option::Option<style::gecko_properties::ComputedValues>, union core::option::Option<style::gecko_properties::ComputedValues>, union core::option::Option<style::gecko_properties::ComputedValues*>, struct style::font_metrics::FontMetricsProvider*, union style::properties::cascade::CascadeMode, selectors::context::QuirksMode, union core::option::Option<style::rule_cache::RuleCache*>, struct style::rule_cache::RuleCacheConditions *, union core::option::Option<style::gecko::wrapper::GeckoElement>) [cascade.rs:e92ff56d2be21676b447c6fbb87b4c4479539bc9 : 170 + 0x408]
14:47:20 INFO -
14:47:20 INFO - Found by: previous frame's frame pointer
14:47:20 INFO -
14:47:20 INFO - Thread 1
14:47:20 INFO - 0 ntdll.dll!NtRemoveIoCompletion + 0x4
14:47:20 INFO -
14:47:20 INFO - Found by: given as instruction pointer in context
14:47:20 INFO - 1 KERNELBASE.dll + 0x17e9e8
14:47:20 INFO -
14:47:20 INFO - Found by: previous frame's frame pointer

<rest truncated for brevity>

Comment 1

[Edwin Takahashi (:egao | infrequent contributor)]

Note, this test has been skipped in https://phabricator.services.mozilla.com/D23744 as part of bug 1530033.

Comment 2

[Kim Moir [:kmoir] ET]

triaging, changing priority since this test has been disabled.

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Kim Moir [:kmoir] ET from comment #2)

triaging, changing priority since this test has been disabled.

These tests were testing something important: If there isn't sufficient runtime stack size, it's easy to crash Firefox with poorly-formatted HTML email that a webmail app, such as Gmail, does not sanitize for excessive nesting.

I think this should be a higher priority than P4.

Comment 4

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69cc22b108e8c578c09c0dcc08a8b11ddc5b88fc

Comment 5

[Henri Sivonen (:hsivonen)]

Try run with better coverage:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=debebb4719a94a5ab8eb307db105b9cd4c5466fd

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1536575 - Stack size on aarch64 Windows should match stack size on x86_64 Windows.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #5)

Try run with better coverage:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=debebb4719a94a5ab8eb307db105b9cd4c5466fd

Does ifneq do a substring match instead of exact match or how can the change make x86_64 Windows fail?

Comment 8

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #7)

(In reply to Henri Sivonen (:hsivonen) from comment #5)

Try run with better coverage:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=debebb4719a94a5ab8eb307db105b9cd4c5466fd

Does ifneq do a substring match instead of exact match or how can the change make x86_64 Windows fail?

Nevermind, I accidentally negated twice.

Comment 9

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68a3aef5bd6d8e7b9ad1ab2f691739ef782b0221

Comment 10

[Henri Sivonen (:hsivonen)]

How can I trigger reftests for aarch64 on try?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ef35b6f16b0
Stack size on aarch64 Windows should match stack size on x86_64 Windows. r=glandium

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1ef35b6f16b0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this something we need to uplift to Beta for Fx67?

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

Is this something we need to uplift to Beta for Fx67?

If we are going to ship 67 for Windows on aarch64, yes. Otherwise, no. Are we going to ship 67 for Windows on aarch64?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I believe so, yes.

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 9056054 [details]
Bug 1536575 - Stack size on aarch64 Windows should match stack size on x86_64 Windows.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: Layout can run out of stack space on Windows on aarch64 and crash when rendering documents that only open font elements without closing them. These can occur in particular in HTML emails that can be served to Firefox by webmail apps, such as Gmail.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The unexpected outcome from the same increase on x86_64 was already resolved for all Windows CPU architectures in bug 1537609, so it's unlikely that there'd be aarch64-specific complications.
• String changes made/needed: None

Comment 17

[Henri Sivonen (:hsivonen)]

Is this code covered by automated tests?: Yes

For clarity: This is covered by tests, but the tests don't run by default.

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9056054 [details]
Bug 1536575 - Stack size on aarch64 Windows should match stack size on x86_64 Windows.

Low risk build system patch for Windows ARM64, uplift approved for 67 beta 10, thanks.

Comment 19

[Edwin Takahashi (:egao | infrequent contributor)]

(In reply to Henri Sivonen (:hsivonen) from comment #10)

How can I trigger reftests for aarch64 on try?

Pardon for late response as I was on PTO.

These tests for windows10-aarch64 can now be run on try with mach try fuzzy, as it should be under windows10-aarch64/opt-reftest-e10s. This requires recent (<2 weeks) old mozilla-central.

Comment 20

[Chris Peterson [:cpeterson]]

(In reply to Henri Sivonen (:hsivonen) from comment #14)

If we are going to ship 67 for Windows on aarch64, yes. Otherwise, no. Are we going to ship 67 for Windows on aarch64?

Yes, Windows for aarch64 will ship in 67.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/611f3e691f0b