1404057 - stylo: thread '<unnamed>' panicked at 'We only ever disable text zoom (in svg:text), never enable it', /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/properties.rs:142969

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 76a26ef7c493.

thread '<unnamed>' panicked at 'We only ever disable text zoom (in svg:text), never enable it', /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/properties.rs:142969
stack backtrace:
   0:     0x7f0dc2aeb483 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hcab99e0793da62c7
   1:     0x7f0dc2ae67a6 - std::sys_common::backtrace::_print::hbfe5b0c7e79c0711
   2:     0x7f0dc2af8b1a - std::panicking::default_hook::{{closure}}::h9ba2c6973907a2be
   3:     0x7f0dc2af871b - std::panicking::default_hook::he4d55e2dd21c3cca
   4:     0x7f0dc2af8f6a - std::panicking::rust_panic_with_hook::ha138c05cd33ad44d
   5:     0x7f0dc295b5ca - std::panicking::begin_panic::ha0986727c75c6797
   6:     0x7f0dc2713948 - style::properties::apply_declarations::h72d8d33cb9cbc0de
   7:     0x7f0dc2712752 - style::properties::cascade::h53c5a34c546c1eb4
   8:     0x7f0dc2b585d5 - <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_style::hbc50856474ba4279
   9:     0x7f0dc2b5917c - <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_style_and_visited::h84c4d3e368475605
  10:     0x7f0dc2b59010 - <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_primary_style::he8a96c4d2e03ceb7
  11:     0x7f0dc2b590a5 - <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::resolve_primary_style::h2323d919e0d8fec1
  12:     0x7f0dc2b58c41 - <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::resolve_style::h0ea046f141b0bc67
  13:     0x7f0dc2b59210 - <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::resolve_style_with_default_parents::{{closure}}::h36e82c4fd5160084
  14:     0x7f0dc2bb632f - style::style_resolver::with_default_parent_styles::h843d207fcc2847c5
  15:     0x7f0dc2b591e8 - <style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::resolve_style_with_default_parents::hdc9bf7ae2d2dee0b
  16:     0x7f0dc2bbc423 - style::traversal::compute_style::he9440e02ab5b4568
  17:     0x7f0dc2cd6f1d - style::traversal::recalc_style_at::h5322d563ff03efd6
  18:     0x7f0dc2cb0eaa - <style::gecko::traversal::RecalcStyleOnly<'recalc> as style::traversal::DomTraversal<style::gecko::wrapper::GeckoElement<'le>>>::process_preorder::hdc0c2db4e0fc1d90
  19:     0x7f0dc2c3e59e - style::driver::traverse_dom::hdebaecf7b2c5112f
  20:     0x7f0dc2338c14 - geckoservo::glue::traverse_subtree::h53594e7648a0d56c
  21:     0x7f0dc2338f68 - Servo_TraverseSubtree
  22:     0x7f0dc09b0c1b - _ZN7mozilla13ServoStyleSet13StyleDocumentENS_19ServoTraversalFlagsE
  23:     0x7f0dc0a92452 - _ZN7mozilla19ServoRestyleManager24DoProcessPendingRestylesENS_19ServoTraversalFlagsE
  24:     0x7f0dc0a93368 - _ZN7mozilla9PresShell27DoFlushPendingNotificationsENS_14ChangesToFlushE
  25:     0x7f0dc0a62d04 - _ZN15nsRefreshDriver4TickElN7mozilla9TimeStampE
  26:     0x7f0dc0a63e1d - _ZN7mozilla18RefreshDriverTimer18TickRefreshDriversElNS_9TimeStampER8nsTArrayI6RefPtrI15nsRefreshDriverEE
  27:     0x7f0dc0a63ef6 - _ZN7mozilla18RefreshDriverTimer4TickElNS_9TimeStampE
  28:     0x7f0dc0a640b0 - _ZN7mozilla23VsyncRefreshDriverTimer26RefreshDriverVsyncObserver17TickRefreshDriverENS_9TimeStampE
  29:     0x7f0dc0a64405 - _ZN7mozilla23VsyncRefreshDriverTimer26RefreshDriverVsyncObserver26ParentProcessVsyncNotifier3RunEv
  30:     0x7f0dbeca9023 - _ZN8nsThread16ProcessNextEventEbPb.part.251
  31:     0x7f0dbeca9d98 - _Z19NS_ProcessNextEventP9nsIThreadb
  32:     0x7f0dbf058a80 - _ZN7mozilla3ipc11MessagePump3RunEPN4base11MessagePump8DelegateE
  33:     0x7f0dbf02e122 - _ZN11MessageLoop11RunInternalEv
  34:     0x7f0dbf02e14e - _ZN11MessageLoop3RunEv
  35:     0x7f0dc085b908 - _ZN14nsBaseAppShell3RunEv
  36:     0x7f0dc1786b9a - _ZN12nsAppStartup3RunEv
  37:     0x7f0dc180c488 - _ZN7XREMain11XRE_mainRunEv
  38:     0x7f0dc180cca0 - _ZN7XREMain8XRE_mainEiPPcRKN7mozilla15BootstrapConfigE
  39:     0x7f0dc180cf85 - _Z8XRE_mainiPPcRKN7mozilla15BootstrapConfigE
  40:           0x40753e - _ZL7do_mainiPPcS0_
  41:           0x406cfe - main
  42:     0x7f0dd045d82f - __libc_start_main
  43:           0x406f60 - <unknown>

Comment 1

[Manish Goregaokar [:manishearth]]

I ... I don't understand how this happens.

Will look at it

Comment 2

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: a304408c275f7f503a078b144351e548e9cbab01
INFO: First bad revision: 3fc46b53b7ee04d6002d9bac5f19f72c899f1444
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a304408c275f7f503a078b144351e548e9cbab01&tochange=3fc46b53b7ee04d6002d9bac5f19f72c899f1444

Comment 3

[Manish Goregaokar [:manishearth]]

Oh, I see. This is just `all` overwriting the zoom property. The document.write threw me off, i thought this was a restyle bug.


Makes me thing, we should probably handle this correctly for lang as well. Neither are actual CSS properties, we just use the same mechanism.

reduced testcase:


<html>
  <title>Testcase, bug 143862</title>
  <head>
<svg><text><t style='all:initial'>aaa</t></text></svg>
  </head>
</html>

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

"all" should probably skip all internal properties? Not sure what Gecko currently does.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> "all" should probably skip all internal properties? Not sure what Gecko
> currently does.

Yep, that's what Gecko does: https://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/layout/style/nsCSSParser.cpp#17414-17421

Comment 6

[Manish Goregaokar [:manishearth]]

Confirmed, this is also broken for `lang`. In the following case, Gecko makes the third character match the second (as it should), stylo makes it match the first.

<head><meta charset="UTF-8"></head> 
神<span lang=ko>神 <span style="all:initial">神</span></span>

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1404057 - stylo: `all` shorthand should not apply to internal properties;

Review commit: https://reviewboard.mozilla.org/r/185924/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185924/

Comment 8

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1404057 - stylo: Add crashtest for text-zoom being reenabled within svg:text;

Review commit: https://reviewboard.mozilla.org/r/185926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185926/

Comment 9

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1404057 - stylo: Add reftest for ensuring lang is not reset by the `all` shorthand ;

Review commit: https://reviewboard.mozilla.org/r/185928/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185928/

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8914581 [details]
Bug 1404057 - stylo: Add crashtest for text-zoom being reenabled within svg:text;

https://reviewboard.mozilla.org/r/185926/#review190920

::: layout/style/crashtests/crashtests.list:243
(Diff revision 1)
>  load 1403465.html
>  load 1403592.html
>  load 1403615.html
>  load 1403712.html
>  load 1404316.html
> +load 1404057.html

You forgot to add the actual test file.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: proposed reftest

I recommend a reftest like this to make the behavior more reliably across platform with the language explicitly specified.

"令" is the character I found with the most significant visual difference in different language (from the table in [1]).

"神" from ko is probably fine as well, but given that Korean doesn't use Han as much as Chinese and Japanese, I would assume a reduced availability of font with that support.

[1] https://en.wikipedia.org/w/index.php?title=Han_unification&oldid=800571415#Examples_of_language-dependent_glyphs

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8914580 [details]
Bug 1404057 - stylo: `all` shorthand should not apply to internal properties;

https://reviewboard.mozilla.org/r/185924/#review190932

I guess this is fine. Although it would be better if we can avoid generating declarations for disabled properties as well, that's probably not a big deal, and can be handled in a separate bug.

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8914580 [details]
Bug 1404057 - stylo: `all` shorthand should not apply to internal properties;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185924/diff/1-2/

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8914581 [details]
Bug 1404057 - stylo: Add crashtest for text-zoom being reenabled within svg:text;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185926/diff/1-2/

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8914582 [details]
Bug 1404057 - stylo: Add reftest for ensuring lang is not reset by the `all` shorthand ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185928/diff/1-2/

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8914582 [details]
Bug 1404057 - stylo: Add reftest for ensuring lang is not reset by the `all` shorthand ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185928/diff/2-3/

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8914581 [details]
Bug 1404057 - stylo: Add crashtest for text-zoom being reenabled within svg:text;

https://reviewboard.mozilla.org/r/185926/#review191246

Assuming this crashes without the patch.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8914582 [details]
Bug 1404057 - stylo: Add reftest for ensuring lang is not reset by the `all` shorthand ;

https://reviewboard.mozilla.org/r/185928/#review191248

Comment 19

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/18734

Comment 20

[Manish Goregaokar [:manishearth]]

https://hg.mozilla.org/integration/autoland/rev/be0e197531f2

should request uplift in a day

Comment 21

[Manish Goregaokar [:manishearth]]

It's not landed yet because of the servo windows builders; comment 20 is for a different bug and was misposted.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38ae97c33bb7
stylo: Add crashtest for text-zoom being reenabled within svg:text; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/24cf61e5a04f
stylo: Add reftest for ensuring lang is not reset by the `all` shorthand ; r=xidorn

Comment 23

[Manish Goregaokar [:manishearth]]

Tree was closed so couldn't land tests earlier. The main commit did land.

https://hg.mozilla.org/mozilla-central/rev/79eea0c0cbb2

Comment 24

[Manish Goregaokar [:manishearth]]

Attachment: Uplift patch

Approval Request Comment
[Feature/Bug causing the regression]: Stylo
[User impact if declined]: May crash
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: The two tests
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minor change, with tests
[String changes made/needed]:

Comment 25

[Manish Goregaokar [:manishearth]]

Attachment: Uplift crashtest

Comment 26

[Manish Goregaokar [:manishearth]]

Attachment: Uplift reftest

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/38ae97c33bb7
https://hg.mozilla.org/mozilla-central/rev/24cf61e5a04f

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916265 [details] [diff] [review]
Uplift patch

Stylo related crash fix, Beta57+

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ad8fe782efec
https://hg.mozilla.org/releases/mozilla-beta/rev/d6cb9fa359e5
https://hg.mozilla.org/releases/mozilla-beta/rev/b95ce26e0412

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1e4eb9ed513
Add back two lines to the crashtest manifest that got accidentally deleted. r=emilio

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/f1e4eb9ed513