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

RESOLVED FIXED in Firefox 67

Status

defect
P4
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: egao, Assigned: hsivonen)

Tracking

({crash, in-triage})

unspecified
mozilla68
ARM64
Windows
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 wontfix, firefox67 fixed, firefox68 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

2 months ago

+++ 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>

Updated

2 months ago
Keywords: in-triage
Reporter

Comment 1

2 months ago

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

triaging, changing priority since this test has been disabled.

Severity: critical → normal
Priority: -- → P4
Assignee

Comment 3

2 months ago

(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.

Summary: windows/aarch64 - REFTEST PROCESS-CRASH | build/tests/reftest/tests/layout/reftests/bugs/256180-1.html, 256180-2.html, 256180-3.html | application crashed → 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
Assignee

Comment 7

2 months ago

(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?

Flags: needinfo?(mh+mozilla)
Assignee

Comment 8

2 months ago

(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.

Assignee

Updated

2 months ago
Flags: needinfo?(mh+mozilla)
Assignee

Comment 10

2 months ago

How can I trigger reftests for aarch64 on try?

Flags: needinfo?(egao)

Comment 11

2 months ago
Pushed by hsivonen@mozilla.com:
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

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → hsivonen

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

Flags: needinfo?(hsivonen)
Assignee

Comment 14

2 months ago

(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?

Flags: needinfo?(hsivonen) → needinfo?(ryanvm)

I believe so, yes.

Flags: needinfo?(ryanvm) → needinfo?(cpeterson)
Assignee

Comment 16

2 months ago

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
Attachment #9056054 - Flags: approval-mozilla-beta?
Assignee

Comment 17

2 months ago

Is this code covered by automated tests?: Yes

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

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.

Attachment #9056054 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reporter

Comment 19

2 months ago

(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.

Flags: needinfo?(egao)

(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.

Flags: needinfo?(cpeterson)
OS: Unspecified → Windows
Hardware: Unspecified → ARM64
You need to log in before you can comment on or make changes to this bug.