Crash [@ mozilla::dom::ResponsiveImageSelector::Content() ]

RESOLVED FIXED in Firefox 38

Status

()

defect
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: humph, Assigned: jdm)

Tracking

({crash, regression, reproducible})

38 Branch
mozilla39
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 unaffected, firefox37 unaffected, firefox38 fixed, firefox39 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

Details

(crash signature, URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
No STR for this, sorry. I had a bunch of tabs open, and browser had been running all day.

Running Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Firefox/38.0
Looks like we might be in responsive mode but mResponsiveSelector is null?
Crash Signature: https://crash-stats.mozilla.com/report/index/4b40ceee-e4db-4f17-9e3f-283ca2150309
Flags: needinfo?(john)
Severity: normal → critical
Crash Signature: [@ mozilla::dom::ResponsiveImageSelector::Content() ] [@ mozilla::dom::HTMLImageElement::AfterSetAttr(int, nsIAtom*, nsAttrValue const*, bool) ]
Keywords: reproducible
OS: Mac OS X → All
Summary: [@ mozilla::dom::ResponsiveImageSelector::Content() ] → Crash [@ mozilla::dom::ResponsiveImageSelector::Content() ]

Comment 5

4 years ago
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e00ed1b014d0&tochange=2bb299df4c33

Suspect: Bug 1017875
(Assignee)

Updated

4 years ago
Assignee: nobody → josh
(Assignee)

Comment 7

4 years ago
There's lots of prior art for checks like this in the file.
Yeah the null check here is proper, and an oversight from when I added InResponsiveMode(), previously that line would've been | if (mResponsiveSelector) |.
Flags: needinfo?(john)
http://www.theguardian.com/uk 
crashing on both Aurora 38.0a2 (2015-03-09) and Nightly 39.0a1 (2015-03-09)
Repeatedly. 

From the user perspective, the page loads and then crashes right away: the tab in nightly, the browser in Aurora.
Attachment #8575024 - Flags: review?(jst) → review+
Duplicate of this bug: 1141664
(Assignee)

Comment 12

4 years ago
I spent some time attempting to make a crashtest for this, but reducing the code on the guardian led me to  8000 lines of minified JS and one very important <aside class="js-related">. I'm not convinced it's worth my time to attempt to further reduce this, and I'll land it without a test.
Posted file guardian.html (obsolete) —
Josh, 


how about ~1300 lines.
1. I reduced the CSS to its minimum, so it would create the crash
2. I reduced the markup It's in the section #highlights that it is happening. I remove what was not necessary. I didn't go into details.
3. I didn't touch inside these two script elements but they are necessary to reproduce the crash.

So we are getting closer I guess.
Flags: needinfo?(josh)
Posted file guardian.html
Further reduction less than 1000 lines. Removed some JS stuff.
Attachment #8575692 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
In my reduction, I did not require any CSS, not any other markup besides the script elements and the <aside>. When I commented out the bootstrap.go(), the crash disappeared, but inlining the corresponding source and calling it manually did not make it reappear.
Flags: needinfo?(josh)
It's caused by unsetting src when InResponsiveMode() returns true

Crash test:

> <img src="foo" srcset="bar">
> <script>
>   document.querySelector("img").removeAttribute('src');
> </script>
https://hg.mozilla.org/mozilla-central/rev/a0f18594c0de
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 19

4 years ago
Ah, thanks. I apparently misread the stack and my tests for _setting_ the attribute weren't working.
(Assignee)

Updated

4 years ago
Blocks: 1142032
(Assignee)

Comment 20

4 years ago
Comment on attachment 8575024 [details] [diff] [review]
Null-check the responsive selector in images

Approval Request Comment
[Feature/regressing bug #]: bug 1067345
[User impact if declined]: Crashes on common websites.
[Describe test coverage new/current, TreeHerder]: crashtest in bug 1142032
[Risks and why]: Simplest possible crash fix.
[String/UUID change made/needed]: None.
Attachment #8575024 - Flags: approval-mozilla-aurora?
Comment on attachment 8575024 [details] [diff] [review]
Null-check the responsive selector in images

Uplifting to aurora since it fixes a regression and seems low risk. 
The test should also get uplifted once it lands.
Attachment #8575024 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1142089
This appears to be fixed on 38 and 39 now. I don't think we need to keep tracking it.

Updated

4 years ago
Duplicate of this bug: 1142163
You need to log in before you can comment on or make changes to this bug.