Closed Bug 1402589 Opened 3 years ago Closed 3 years ago

stylo: Crash in nsIDocument::AdoptNode

Categories

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

58 Branch
Unspecified
Windows 10
defect

Tracking

()

RESOLVED WORKSFORME
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- ?

People

(Reporter: calixte, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, Whiteboard: [clouseau])

Crash Data

This bug was filed from the Socorro interface and is 
report bp-e89d8ec4-9618-477d-ae9a-159e30170922.
=============================================================

There are 17 crashes in nightly 58 starting with buildid 20170921220243. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1401792.

[1] https://hg.mozilla.org/mozilla-central/rev?node=ae92b4765e63b78e24dfdf89d2c0d8c404a56da8
Flags: needinfo?(xidorn+moz)
That assertion was added to stop adopting element with style data across different style backend (from gecko to stylo or verse vice). If we don't stop that, it can lead to security issue. And this assertion was added mostly for diagnosis.

In theory, content page cannot do such movement, because after bug 1400540, they should not be able to create a page backed by Gecko style system at all when Stylo is enabled. But a content script from browser or extension can still create such document and move elements between different style backend.

It would be helpful if we can get the JS stack from the reports and see what browser script or extension script tries to do that, so that we can analyse what could be the best way forward.
Flags: needinfo?(xidorn+moz)
Priority: -- → P2
You could call AppendAppNotesToCrashReport just before the MOZ_RELEASE_ASSERT to add some extra information to the crash report.  Although probably you shouldn't add a JS stack trace, since that would be PII (as it would show document URLs).  Maybe you could get a JS stack trace and sanitize it (censor all URLs other than about: / chrome: URLs)?

You could also look at what the old and new document URIs are, again sanitized, and if see if it makes sense or provides a clue.
Can someone look at some bug report and see if there is any meaningful urls in it?
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> Can someone look at some bug report and see if there is any meaningful urls
> in it?

I mean, crash reports :P
There are a few about:blanks, a couple of Wikipedia pages, a couple of Google search result pages.  The Wikipedia and Google search result pages are probably the same user.
I'll work on a patch for collecting a bit more data for analysis tomorrow. ni? myself for that.
Flags: needinfo?(xidorn+moz)
And for my own info, some discussion with heycam on IRC: http://logs.glob.uno/?c=mozilla%23servo#c757815
Assignee: nobody → xidorn+moz
Depends on: 1403024
Will use bug 1403024 for adding the diagnosis information. Once we get enough useful data, we can proceed with the real issue in this bug.
Flags: needinfo?(xidorn+moz)
From a crash report <bp-3efa2cd5-8aa3-4cd6-a948-468780170927>;

 Was just clicking reply to an email in a gmail pop-up when the pop-up window crashed.
Per bug 1404020 comment 14, I think this isn't a blocking issue.
Priority: P2 → P3
Unassigning myself given that there isn't anything actionable at the moment.
Assignee: xidorn+moz → nobody
Deprioritize this bug given bug 1404020 comment 14.
Priority: P3 → P4
Given comments #10 and #12, let's not block 58 on this.
Now I was hit by this bp-a813ef5c-1a17-42c3-8ad0-cc5aa0180115.

What I did (as far as I remember), I changed layout.css.servo.enabled false for testing (layout.css.servo.chrome.enabled is true), after a while (about an hour later? I did forget I changed the pref), I noticed there was a notification on irccloud tab (it's a pinned tab), then I switched to the tab, then crashed. :/
Is this bug report still relevant?
Flags: needinfo?(xidorn+moz)
No, I don't think this bug is relevant anymore. It was just a unfortunate case when we have both style systems enabled.

Given that even 60esr has only stylo enabled, this is no longer relevant.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(xidorn+moz)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.