Closed Bug 1141260 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

38 Branch
x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- unaffected
firefox37 --- unaffected
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: humph, Assigned: jdm)

References

()

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files, 1 obsolete file)

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() ]
[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: nobody → josh
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+
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.
Attached 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)
Attached file guardian.html
Further reduction less than 1000 lines. Removed some JS stuff.
Attachment #8575692 - Attachment is obsolete: true
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Ah, thanks. I apparently misread the stack and my tests for _setting_ the attribute weren't working.
Blocks: 1142032
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.
Duplicate of this bug: 1142163
You need to log in before you can comment on or make changes to this bug.