Closed Bug 1579788 Opened 5 years ago Closed 5 years ago

Crash in [@ mozilla::RestyleManager::ContentStateChanged]

Categories

(Core :: Layout: Form Controls, defect, P2)

Product:
Core
Component:
Layout: Form Controls
Version:
70 Branch
Platform:
Desktop
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 + fixed

People

(Reporter: ernstp, Assigned: emilio)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, testcase)

Crash Data

Attachments

(3 files)

Bug 1579788 - Downgrade a few assertions in beta / release as to avoid crashing there.
47 bytes, text/x-phabricator-request
lizzard
: approval-mozilla-beta+
Details | Review
testcase.html
346 bytes, text/html
Details
Bug 1579788 - Don't update validity state when the JS engine fails to execute the pattern.
47 bytes, text/x-phabricator-request
Details | Review

This bug is for crash report bp-eb2cbcc2-a3da-4304-a52c-ee7a90190906.

When I upgraded to 70b3 this started happening 100% when trying to log in to Artifactory (5.11.1).

Top 10 frames of crashing thread:

0 libxul.so mozilla::RestyleManager::ContentStateChanged layout/base/RestyleManager.cpp:3227
1 libxul.so mozilla::dom::Element::UpdateState layout/base/PresShell.cpp:4254
2 libxul.so mozilla::dom::HTMLFormElement::UpdateValidity dom/html/HTMLFormElement.cpp:1853
3 libxul.so mozilla::dom::HTMLInputElement::UpdateAllValidityStatesButNotElementState dom/html/HTMLInputElement.cpp:6712
4 libxul.so non-virtual thunk to mozilla::dom::HTMLInputElement::OnValueChanged dom/html/HTMLInputElement.cpp
5 libxul.so nsTextEditorState::SetValue dom/html/nsTextEditorState.cpp:2508
6 libxul.so nsTextEditorState::UnbindFromFrame dom/html/nsTextEditorState.cpp:2045
7 libxul.so nsTextControlFrame::DestroyFrom layout/forms/nsTextControlFrame.cpp:137
8 libxul.so nsContainerFrame::DestroyFrom layout/generic/nsContainerFrame.cpp:215
9 libxul.so nsLineBox::DeleteLineList layout/generic/nsLineBox.cpp:386

[Tracking Requested - why for this release]: Seems like a recent regression.

I don't know what Artifactory is, can you point to a URL I could use to reproduce this?

We could mitigate this by downgrading the assertion... but that's not great, and if you just started experimenting it it seems like a regression (I made those assertions release asserts in bug 1528644).

tracking-firefox70: --- → ?
Flags: needinfo?(ernstp)

Ah sorry, Artifactory is a quite common software that's mostly used by companies in-house to store files and have repositories etc., https://jfrog.com/artifactory/
And this crash happens then in it's web based UI of course...

Ah here's a public one, let's see... https://repo.jenkins-ci.org/webapp/
The issue is when you log in so you'll need an account of course.

Flags: needinfo?(ernstp)

But https://repo.jenkins-ci.org/webapp/ seems to be running Artifactory 6 and I can't reproduce the bug there...

that's unfortunate :(

Any chance saving the page where you crash reproduces it? If so attaching it here would be helpful...

I tried saving the page but it's full of javascript and very dynamic in it's nature it seems, and doesn't work when saved...

(Still happens on 70.0b5)

Is there a way to search for all crash reports with a longer signature match?
There are many different ContentStateChanged crashes right now it seems...

But here's another random I found with UpdateAllValidityStatesButNotElementState
https://crash-stats.mozilla.org/report/index/e84f8eba-84ea-4793-ba66-9bdc80190911

Tyson has any of the fuzzers hit this?

Flags: needinfo?(twsmith)

I think I have a match with 4 test cases. I'll reduce and attach asap.

This is the only short-term fix for now until we fix editor or find a
test-case...

This will keep asserting on Nightly, but the correctness issue it'd show in
release (some pseudo-classes not matching) is better than crashing.

I'll submit a patch to avoid crashing in beta / release, but I really really want to get to the bottom of this...

(In reply to Tyson Smith [:tsmith] from comment #9)

I think I have a match with 4 test cases. I'll reduce and attach asap.

Oh, that's awesome! <3

Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2

Comment on attachment 9092173 [details]
Bug 1579788 - Downgrade a few assertions in beta / release as to avoid crashing there.

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: We have no concrete STR yet, but there are crashes in the wild and this patch will stop crashing instead showing some minor correctness issues (like pseudo-classes not correctly matching).
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Just making an assertion not assert on release. This used to be a debug assertion not long ago, and has no security implications, only correctness implications.
  • String changes made/needed: none
Attachment #9092173 - Flags: approval-mozilla-beta?

Comment on attachment 9092173 [details]
Bug 1579788 - Downgrade a few assertions in beta / release as to avoid crashing there.

Avoid a crash in 70 release, let's uplift for beta 6.

Attachment #9092173 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached file testcase.html —
Flags: needinfo?(twsmith)
Blocks: domino
Keywords: testcase
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Keywords: leave-open
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/667132500392
Downgrade a few assertions in beta / release as to avoid crashing there. r=masayuki

Mozregression says bug 1525509 regressed the test-case, as expected, since it was the bug which turned the MOZ_ASSERT to MOZ_RELEASE_ASSERT... But it was a longstanding issue...

Regressed by: 1525509

I wrote a fix for the fuzzer test-case, but I suspect it's not the issue we're seeing in the wild.

Worth fixing anyhow.

Flags: needinfo?(emilio)

When we call nsContentUtils::IsPatternMatching, we swallow JS engine errors
unconditionally returning true.

This is bad, because if it happens in one of the value sets that arguably
shouldn't change the state of the element, we end up returning an arbitrary
value (true) which may or may not match the previous state of the element.

Handle error explicitly instead, by not updating the state.

Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36d5acf94959
Don't update validity state when the JS engine fails to execute the pattern. r=smaug

Emilio: Do we still need leave-open on this?

Flags: needinfo?(emilio)

There are still some crashes on Nightly... All seem to come from HTMLInputElement / the editing code. Tyson, any other test-case for this? :)

Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(emilio) → needinfo?(twsmith)
Resolution: --- → FIXED

(In reply to Emilio Cobos Álvarez (:emilio) from comment #26)

There are still some crashes on Nightly... All seem to come from HTMLInputElement / the editing code. Tyson, any other test-case for this? :)

But we should probably file different bugs for that, I mean.

I don't see any new test cases for this issue. I'll let the fuzzers run over the weekend and see what we shakeout.

Flags: needinfo?(twsmith)

+1 to filing a new bug for future testcases.

status-firefox67: --- → wontfix
status-firefox68: --- → wontfix
status-firefox69: --- → wontfix
status-firefox71: affected → ---
status-firefox-esr60: --- → unaffected
status-firefox-esr68: --- → wontfix
Flags: in-testsuite+
Target Milestone: --- → mozilla70
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: