Closed Bug 1370968 Opened 2 years ago Closed 2 years ago

Crash in nsLayoutUtils::FindNearestCommonAncestorFrame

Categories

(Core :: Layout, defect, critical)

55 Branch
Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: calixte, Assigned: tschneider)

References

(Blocks 1 open bug)

Details

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

Crash Data

Attachments

(2 files, 3 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-489abd7b-dce1-4fed-a01e-d1ca20170607.
=============================================================

There is 2 crashes (from the same installation) in nightly 55 with buildid  	20170607030206. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1359318.

[1] https://hg.mozilla.org/mozilla-central/rev?node=279a08ffe577ec62a6b9d5ca76fe25bd117dd2b3
Flags: needinfo?(tschneider)
Attached patch Crashtest (obsolete) — Splinter Review
Flags: needinfo?(tschneider)
Problem here was that a target within an XUL page, loaded via an iframe, was observed and the code to calculate the intersection expected the target having a root scroll frame. We should guard properly against any cases of NULL when using this API within a XUL context.
Attachment #8875477 - Flags: review?(mstange)
Comment on attachment 8875477 [details] [diff] [review]
Guard against possible NULL values when IntersecionObserver API is used in XUL pages

Review of attachment 8875477 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +417,5 @@
> +        if (presContext) {
> +          nsCOMPtr<nsIPresShell> presShell = presContext->PresShell();
> +          if (presShell) {
> +            nsIFrame* rootScrollFrame = presShell->GetRootScrollFrame();
> +            if (rootScrollFrame) {

The rootScrollFrame check should be the only one that's needed. A frame always has a PresContext and a PresContext always has a PresShell. (Except maybe during frame / prescontext destruction.)

The general rule is: Getters that start with the word "Get" can return null, those that don't can not.
Addressed review comment.
Attachment #8875477 - Attachment is obsolete: true
Attachment #8875477 - Flags: review?(mstange)
Attachment #8875492 - Flags: review?(mstange)
Attachment #8875492 - Flags: review?(mstange) → review+
Attached patch CrashtestSplinter Review
Attachment #8875435 - Attachment is obsolete: true
Keywords: checkin-needed
Assignee: nobody → tschneider
Flags: in-testsuite+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/217c7653e6ec
Guard against possible NULL values when IntersecionObserver API is used in XUL pages. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c606fafb195
Test for crash in nsLayoutUtils::FindNearestCommonAncestorFrame. r=mstange
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/217c7653e6ec
https://hg.mozilla.org/mozilla-central/rev/5c606fafb195
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.