Closed Bug 1406222 Opened 7 years ago Closed 7 years ago

stylo: thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value' in [@ style::stylist::Stylist::compute_style_with_inputs]

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

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

People

(Reporter: tsmith, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(2 files)

test_case.html
584 bytes, text/html
Details
Bug 1406222 tests.
59 bytes, text/x-review-board-request
heycam
: review+
ritu
: approval-mozilla-beta+
Details
Attached file test_case.html — 
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/option.rs:335

stack backtrace:
   0:     0x7fe8e65909b3 - std::sys::imp::backtrace::tracing::imp::unwind_backtrace::hcab99e0793da62c7
                               at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x7fe8e658d979 - std::panicking::default_hook::{{closure}}::h9ba2c6973907a2be
                               at /checkout/src/libstd/sys_common/backtrace.rs:71
                               at /checkout/src/libstd/sys_common/backtrace.rs:60
                               at /checkout/src/libstd/panicking.rs:355
   2:     0x7fe8e658cda0 - std::panicking::default_hook::he4d55e2dd21c3cca
                               at /checkout/src/libstd/panicking.rs:371
   3:     0x7fe8e658c8c5 - std::panicking::rust_panic_with_hook::ha138c05cd33ad44d
                               at /checkout/src/libstd/panicking.rs:549
   4:     0x7fe8e658c7df - std::panicking::begin_panic::hcdbfa35c94142fa2
                               at /checkout/src/libstd/panicking.rs:511
   5:     0x7fe8e658c749 - std::panicking::begin_panic_fmt::hc09fe500d9b7be81
                               at /checkout/src/libstd/panicking.rs:495
   6:     0x7fe8e659ae86 - core::panicking::panic_fmt::h883a028e9f4b4457
                               at /checkout/src/libstd/panicking.rs:471
   7:     0x7fe8e659cc95 - core::panicking::panic::hdb3cf3207dda37bb
                               at /checkout/src/libcore/panicking.rs:49
   8:     0x7fe8e64eac2e - style::stylist::Stylist::compute_style_with_inputs::ha1cd7ecde12a429a
Flags: in-testsuite?
INFO: Last good revision: 91a488108e10bfd4df90ccf8b738ae5c4a0f0dc1
INFO: First bad revision: ab3c85d4d199c903f6359e276def141d67a000d7
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=91a488108e10bfd4df90ccf8b738ae5c4a0f0dc1&tochange=ab3c85d4d199c903f6359e276def141d67a000d7
Blocks: 1324619
Has Regression Range: --- → yes
status-firefox56: --- → disabled
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(bzbarsky)
Priority: -- → P1
Ah, I had misunderstood the way rulenodes worked on things that match no rules.  I thought they would be ruletree root, not None.

We have a ComputedValues for a textframe.  It was created via ResolveStyleForTextOrFirstLetterContinuation calling Servo_ComputedValues_Inherit, as expected.  In the case when there is a parent style context (as here), Servo_ComputedValues_Inherit does StyleBuilder::for_inheritance, which does Self::new passing None for "rules".  So it creates a StyleBuilder with a None "rules" member.

When StyleBuilder::build is then called, it just passes self.rules as the style.  So the textframe ends up with a ComputedValues with a None "rules" member.

OK, so now we Servo_ReparentStyle on that textframe's style.  That does CascadeInputs::new_from_style() on the given style, which sets up the CascadeInputs with None as "rules".  We pass that to Stylist::compute_style_with_inputs.  This sees that parent_style.visited_style().isSome() and does:

            let rule_node = match inputs.visited_rules.as_ref() {
                Some(rules) => rules,
                None => inputs.rules.as_ref().unwrap(),
            };

OK, but inputs.visited_rules and inputs.rules are both None, so this panics.  The comment I had about how we must have either visited or unvisited rules in inputs is just wrong.
Assignee: nobody → bzbarsky
Flags: needinfo?(bzbarsky)
What I don't understand is why this doesn't crash any time we have a block-level <a> that has ::first-line.  But I've checked, and it does not...

Maybe we end up giving the text visited rules if it starts off with a link parent.  But then making the parent a link dynamically would still cause problems, and it doesn't on its own.  And for some reason the column style on the <a> is needed.  Here's what I have so far as a minimal testcase:

  <!DOCTYPE html>
  <style>
  a { display: block; }
  a { columns: 0px; }
  a::first-line {}
  </style>
  <script>
  onload = function() {
    document.body.offsetWidth;
    document.body.style.color = "green";
    document.body.offsetWidth;
    document.querySelector("a").href = "Something";
  }
  </script>
  <a>Some text</a>

If I take out the column style it, does not crash.  If I take out the restyle on the body before I give the <a> an href, it does not crash...
Hmm.  Maybe that's basically due to bug 1406254 and us giving the ::first-line a bogus (e.g. nonexistent?) if-visited style context normally or something...
But that wouldn't explain the columns bits, really.
I'd still love it if someone can explain why the minimal testcase can't become even more minimal!
tracking-firefox57: --- → ?
tracking-firefox58: --- → ?
Tracking 57/58+ for this stylo issue.
tracking-firefox57: ? → +
tracking-firefox58: ? → +
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d5f1b7ca130c -d 25db67fda015: rebasing 424525:d5f1b7ca130c "Bug 1406222 tests. r=heycam" (tip)
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment on attachment 8915809 [details]
Bug 1406222 tests.

Approval Request Comment
[Feature/Bug causing the regression]: Stylo ::first-line support
[User impact if declined]: Crashes in some cases.
[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]: None.
[Is the change risky?]: Shouldn't be.
[Why is the change risky/not risky?]: Just actually following the stylo
   invariants.
[String changes made/needed]: None.
Attachment #8915809 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/51e32c0d6f6e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8915809 [details]
Bug 1406222 tests.

Stylo related crash fix, beta57+
Attachment #8915809 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Test only fixes (as is the case in the nom'd patch) don't need a relman approval.
Comment on attachment 8915809 [details]
Bug 1406222 tests.

RyanVM these are test-only changes so I removed the beta nom flag.
Flags: needinfo?(ryanvm)
Attachment #8915809 - Flags: approval-mozilla-beta+
Comment 11 has the non-test fix.
Flags: needinfo?(ryanvm) → needinfo?(rkothari)
I assume you'd like to uplift the crash fix, not just the tests?

In other bugs, I've used a manually-entered attachment containing a link to the Servo side changeset on autoland for this purpose.  Then you can use that to request uplift.
Ok, will retag beta+.
Flags: needinfo?(rkothari)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: