1337740 - Crash in nsFrame::BoxReflow with "Firefox 2, the theme, reloaded" v1.0.13

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[rctgamer3]

This bug was filed from the Socorro interface and is 
report bp-c14f3762-b1fe-441c-ae41-4154b2170208.
=============================================================
Received user reports of Firefox crashing on the latest version of my "Firefox 2, the theme, reloaded" Complete Theme.
https://addons.mozilla.org/addon/6898/

Possible STR differ per report. Some say it crashes on pages with a video player, some on pages without. Has 280 crash reports already, majority is on Windows 7, with Windows 10 second. Most commonly happens with 51.0.1.

Comment 2

[rctgamer3]

This crash seems to be caused by a namespace change in videocontrols.css introduced in bug 1271765.

Submitted an update to my theme that reverts to the old videocontrols.css. Added an override in chrome.manifest for 53 and up. (Yeah, my fault for not noticing that the new media controls were targeted for 53.)

omni.ja\chrome\toolkit\skin\classic\global\media\videocontrols.css
- @namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
- @namespace html url("http://www.w3.org/1999/xhtml");
---
+ @namespace xul url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
+ @namespace url("http://www.w3.org/1999/xhtml");

STR: Install version 1.0.13 or 1.0.13rc1 of the theme and navigate to a page with video/audio, for example the video in the Add-ons Manager or the Mozilla mission video.

Comment 3

[Loic]

Already fixed:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ec7fb4f14d3ec23ded7eea40ff49ebbcbec6bde1&tochange=8d2eecb7ea5a16e02862dd326ce4519082ce9901

Ray Lin — Bug 1271765 - Part 1: Remove XUL specific reflow code of video control. r=dholbert

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to rctgamer3 from comment #2)
> This crash seems to be caused by a namespace change in videocontrols.css
> introduced in bug 1271765.
[...]
(In reply to Loic from comment #3)
> Already fixed:
> Ray Lin — Bug 1271765 - Part 1: Remove XUL specific reflow code of video control. r=dholbert

I'm not sure I understand. Comment 2 says that bug's changes *caused* the problem, whereas Comment 3 says the same bug's first patch *fixed* the problem.

Do we know when this actually regressed? (And am I correct in interpreting comment 3 to mean this isn't an issue in current Nightly?)

Comment 5

[Loic]

It's fixed in FF53/54, I can't reproduce the crash with builds after bug 1271765 patched.

Comment 6

[:Gijs (he/him)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (In reply to rctgamer3 from comment #2)
> > This crash seems to be caused by a namespace change in videocontrols.css
> > introduced in bug 1271765.
> [...]
> (In reply to Loic from comment #3)
> > Already fixed:
> > Ray Lin — Bug 1271765 - Part 1: Remove XUL specific reflow code of video control. r=dholbert
> 
> I'm not sure I understand. Comment 2 says that bug's changes *caused* the
> problem, whereas Comment 3 says the same bug's first patch *fixed* the
> problem.
> 
> Do we know when this actually regressed? (And am I correct in interpreting
> comment 3 to mean this isn't an issue in current Nightly?)

The theme referenced in comment #0 and comment #2 is a custom 'full' theme for Firefox, and so it had its own copy of videocontrols.css based on current 53/trunk, and using that copy with 52 or 51 (or below, I guess) crashes with similar crashes as we saw in bug 1271765 before changing the layout code there to deal with those crashes. IOW, this is basically implying "the layout changes (but not necessarily the CSS changes to the video controls themselves) from bug 1271765 should be in 52/51". I mean, that or wontfix - comment #2 indicates the custom theme has already been updated, and I don't know if the layout changes from 1271765 would cause any issues with the 'previous' video controls.

Does that clarify?

Comment 7

[Ryan Trunten]

hey "gamer", just wanted to say thank you for the following up and dedication!

Comment 8

[Ryan Trunten]

To further clarify, upon doing some research...

A change in a previous version of said theme did not agree with previous Firefox versions.

https://addons.mozilla.org/en-US/firefox/addon/firefox-2-theme-for-firefox-3x/versions/

Current version of theme as of today appears to have resolved the problem.

Problem was indeed related to video player controls with HTML5 video.

Comment 9

[Daniel Holbert [:dholbert]]

Thanks Gijs - that clarifies indeed.
> IOW, this is basically implying "the layout changes (but not necessarily
> the CSS changes to the video controls themselves) from bug 1271765 should
> be in 52/51".
> I mean, that or wontfix - comment #2 indicates the custom theme has
> already been updated, and I don't know if the layout changes from
> 1271765 would cause any issues with the 'previous' video controls.

I don't think uplifting bug 1271765 is an option.  It makes sense to fix this in the addon itself (and as you note, I think it already has been?)  --> reclassifying as Tech Evang | Addons.

(The fact that there's a crash in nsFrame::BoxReflow does make this *look* like a layout bug, but I believe that's just what happens when you mix XUL & non-XUL frames.  And in this case, the videocontrols.css changes that this addon pulled in were causing some XUL elements to be styled as non-XUL frames, despite being in a XUL hierarchy.)

Comment 10

[Daniel Holbert [:dholbert]]

I think we can call this FIXED (in the add-on) per comment 8 ("Current version of theme as of today appears to have resolved the problem")

Indeed, the add-on's Version Information blurb for 1.0.14 (latest version, released today) says that it fixes this crash:
> 1.0.14: Fixed undocumented crash when loading a page with HTML5 video/audio
> caused by namespace change in Firefox 53 (bug 1337740)

Comment 11

[[:philipp]]

the signature is spiking up since February 5th & correlations from crash reports point to those other themes as well: 
https://addons.mozilla.org/firefox/addon/qute-5-custom-mod/
https://addons.mozilla.org/firefox/addon/qute-6/

Comment 12

[Daniel Holbert [:dholbert]]

Thanks for the heads-up, philipp.  Might be a similar problem there -- do you have a sample crash report with one of those add-ons?

I do see videocontrols.css files inside of those addons' source, so it makes some sense. However, I can't reproduce the crash locally with the latest version of the qute-5-custom-mod addon. I tested Nightly 54, Aurora 53, and Release 51.01, loading these data URLs:
 data:text/html,<video controls>
 data:text/html,<audio controls>

Comment 13

[[:philipp]]

ok, i am sorry and have to backtrack on comment #11, as it was mistaken (a hiccup in the correlations tool for the signature caused outdated results for the beta channel) - these crashes in the qute theme were already happening back in january and caused the spike in this signature back then, like https://crash-stats.mozilla.com/report/index/0a207ac1-1b0f-4fce-9d4f-15e882170119

this already got fixed by the addon author though:
https://addons.mozilla.org/firefox/addon/qute-6/versions/?page=1#version-2.0.5.1

Comment 14

[Daniel Holbert [:dholbert]]

Ah, yes -- in the release notes you linked there, "* Browser: Fix a crash for video controls" -- that must've been what addressed this there.

Thanks for the followup!