Closed Bug 1404057 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: manishearth)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(8 files)

trigger.html
224 bytes, text/html
Details
Bug 1404057 - stylo: `all` shorthand should not apply to internal properties;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1404057 - stylo: Add crashtest for text-zoom being reenabled within svg:text;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1404057 - stylo: Add reftest for ensuring lang is not reset by the `all` shorthand ;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
proposed reftest
1.99 KB, patch
Details | Diff | Splinter Review
Uplift patch
1.77 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
Uplift crashtest
1.09 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
Uplift reftest
2.02 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file 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>
Flags: in-testsuite?
Flags: needinfo?(manishearth)
Priority: -- → P2
Assignee: nobody → manishearth
I ... I don't understand how this happens.

Will look at it
Flags: needinfo?(manishearth)
INFO: Last good revision: a304408c275f7f503a078b144351e548e9cbab01
INFO: First bad revision: 3fc46b53b7ee04d6002d9bac5f19f72c899f1444
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a304408c275f7f503a078b144351e548e9cbab01&tochange=3fc46b53b7ee04d6002d9bac5f19f72c899f1444
Blocks: 1358688
Has Regression Range: --- → yes
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
Version: unspecified → 57 Branch
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>
"all" should probably skip all internal properties? Not sure what Gecko currently does.
(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
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 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.
Attachment #8914581 - Flags: review?(xidorn+moz)
Attached patch proposed reftest — — Splinter Review 
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 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.
Attachment #8914580 - Flags: review?(xidorn+moz) → review+
Status: NEW → ASSIGNED
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.
Attachment #8914581 - Flags: review?(xidorn+moz) → review+
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
Attachment #8914582 - Flags: review?(xidorn+moz) → review+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
It's not landed yet because of the servo windows builders; comment 20 is for a different bug and was misposted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by manishearth@gmail.com:
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
Tree was closed so couldn't land tests earlier. The main commit did land.

https://hg.mozilla.org/mozilla-central/rev/79eea0c0cbb2
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Attached patch Uplift patch — — Splinter Review 
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]:
Attachment #8916265 - Flags: approval-mozilla-beta?
Attached patch Uplift crashtest — — Splinter Review 

        Attachment #8916267 -
        Flags: approval-mozilla-beta?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Uplift reftest
        — 
          — Splinter Review
      

    


  

        Attachment #8916268 -
        Flags: approval-mozilla-beta?

          status-firefox58:
          affected → fixed
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla58

    
    

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

Stylo related crash fix, Beta57+

        Attachment #8916265 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

        Attachment #8916267 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

        Attachment #8916268 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f1e4eb9ed513
Add back two lines to the crashtest manifest that got accidentally deleted. r=emilio

          You need to log in
          before you can comment on or make changes to this bug.