Closed Bug 1328423 Opened 3 years ago Closed 3 years ago

Back out bug 1308039 (painting during GC)

Categories

(Core :: JavaScript: GC, defect)

52 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox53 + fixed

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Today I went through all the regressions that have accumulated from bug 1308039 and I feel pretty strongly that we should back it out. It didn't have any effect that I know of on telemetry. Perhaps we could land it later on, but we shouldn't do so until we actually fix all the places where we call into the JS engine from painting. The current list of unfixed crashers is:
bug 1310745, bug 1311841, bug 1311843, bug 1316683, bug 1325813, bug 1327965, bug 1314764, bug 1310335, bug 1325813.
In addition, I've found some crashes that have been lumped in with these that were never filed.

Note that we can still keep the "paint while JS is running patch" (bug 1279086). That patch actually does seem to improve things on telemetry. In addition, these crashes mostly don't affect that bug. The assertions come in two forms:
1. Briefly entering the JS engine without running any code. It's totally fine to do this when content JS is running.
2. Running some chrome code. This is a little scarier to do when content JS is running. However, there are all sorts of other places where we run chrome code while content JS is running, so I don't feel that this is a major concern.

Brad, Mike, how do you feel about a backout?
Flags: needinfo?(mconley)
Flags: needinfo?(blassey.bugs)
Version: unspecified → 52 Branch
The data I have displayed at http://mikeconley.github.io/bug1310250 [1] shows that there wasn't much of an impact when the GC interruption stuff landed. This plan is okay for me. I'll keep an eye on the graphs if / when we do this to make sure there are no surprises.

[1]: Please excuse the horrific label collisions
Flags: needinfo?(mconley)
If this didn't improve the spinners and causes crashes, I think a back out is the right thing to do.
Flags: needinfo?(blassey.bugs)
Tracking 52/53+ for the backout.
Please also remove the MOZ_RELEASE_ASSERT in ScriptedNotificationObserver::Notify() at the same time.
And the one in nsContentUtils::IsPatternMatching(). I don't think they'll be needed any more.
Depends on: 1310335
Here is the backout:
Backout by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9c1a72a8ec
Back out - Preempt GC to paint (a=backout)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0001674978a5
Back out - GC interrupt callbacks (a=backout)
https://hg.mozilla.org/integration/mozilla-inbound/rev/7faa70924168
Back out - Add AutoAssertOnBarrier to painting code (a=backout)

However, I would like to add some new assertions to check that we don't have more problems.
The new assertion we need is that we shouldn't run content JS during painting. I think we need a new JS engine assertion for this.
Attachment #8825990 - Flags: review?(sphink)
This patch weakens the tab switch assertion so that we allow anything except content JS execution during tab switching.
Attachment #8825991 - Flags: review?(dvander)
This patch is pretty strong. It makes sure we don't run content JS during any paints except those that might run MozAfterPaint events. It looks like this includes all paints in a regular browser window.
Attachment #8825992 - Flags: review?(dvander)
With the previous patch (asserting that we don't run content JS during painting), I was getting assertions on Windows. The problem was this:

- We run XUL tests with content privileges during testing (both mochitests and reftests).
- These tests include a XUL <menulist> in the document.
- When the Windows theming code paints a <menulist>, it checks if they're editable to decide how they should be styled.
- Checking the editable property ends up calling into the XBL binding for <menulist> to look at the "editable" property.
- This causes JS to run (during painting) and this JS runs with the document's content principal.

I don't think we normally allow XUL in content, so I think we only care about styling chrome XUL correctly. Therefore, I changed the theming code to only check the editable property for chrome documents. That works around the assertion.
Attachment #8826061 - Flags: review?(jmathies)
Attachment #8825990 - Flags: review?(sphink) → review+
Attachment #8825991 - Flags: review?(dvander) → review+
Attachment #8825992 - Flags: review?(dvander) → review+
Attachment #8826061 - Flags: review?(jmathies) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b48a84c99dfc
Add AutoAssertNoContentJS assertion (r=sfink)
https://hg.mozilla.org/integration/mozilla-inbound/rev/dffce96fd4a0
Switch tab switch assertion to use AutoAssertNoContentJS (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/672f1b3720eb
Add AutoAssertNoContentJS to PresShell::Paint (r=dvander)
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5f238456ce
Avoid calling into JS from Windows theming code (r=jimm)
Depends on: 1331966
Blocks: 1310335
No longer depends on: 1310335
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(wmccloskey)
Duplicate of this bug: 1314764
Comment on attachment 8825991 [details] [diff] [review]
Switch tab switch assertion

Since the uplift deadline is near, I'd only like to backport the part of this patch that removes the old assertion. I won't be adding the new assertion. The risk is very low since we're just taking out an assertion.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1308039
[User impact if declined]: release assertions when switching tabs
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[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?]: no
[Why is the change risky/not risky?]: It just removes an assertion
[String changes made/needed]: none
Flags: needinfo?(wmccloskey)
Attachment #8825991 - Flags: approval-mozilla-aurora?
Comment on attachment 8825991 [details] [diff] [review]
Switch tab switch assertion

remove an assertion about gc on tab switch
Attachment #8825991 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I think things got a little mucked up in communication here. The backout of bug 1308039 was already landed on Aurora a week ago by Bill:
https://hg.mozilla.org/releases/mozilla-aurora/rev/fc150c017ac2a53b02a05d007d9c61bcd841c3ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/f312b22c2ec54129f8cfc938ab4fd21c7551e48b
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c681a167a5e3ce19021604e1b3661d2418b4966

IIUC, that's all that was needed here. I've backed out the uplift from comment 17 as it caused bustage and I'm pretty certain from Bill's comment that it wasn't actually meant to land on Aurora anyway. Bill, can you please confirm that I've got all this right?
https://hg.mozilla.org/releases/mozilla-aurora/rev/34706248bfb88693475f07829a96d0eaf6e11b57
Flags: needinfo?(wmccloskey)
Sorry, this wasn't quite right. I pushed the correct patch.
https://hg.mozilla.org/releases/mozilla-aurora/rev/a891cb2fa60257c3a8cb84bb0356fba508e42f3e

Did this make it in time, Ryan?
Flags: needinfo?(wmccloskey) → needinfo?(ryanvm)
Yep, thanks!
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.