Closed Bug 1450208 Opened 7 years ago Closed 7 years ago

Crash in static void ExtractRectFromOffset

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Platform:
Unspecified
Windows 10
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + verified
firefox61 --- verified

People

(Reporter: calixte, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

Bug 1450208: Change nsRange::ExtractRectFromOffset to use simpler, hopefully safer logic to determine whether text is vertical.
59 bytes, text/x-review-board-request
bzbarsky
: review+
lizzard
: approval-mozilla-beta+
Details
This bug was filed from the Socorro interface and is report bp-01ddbaf5-8008-491e-8ae5-679800180330. ============================================================= Top 10 frames of crashing thread: 0 xul.dll static void ExtractRectFromOffset dom/base/nsRange.cpp:2922 1 xul.dll nsRange::CollectClientRectsAndText dom/base/nsRange.cpp:3091 2 xul.dll nsRange::GetClientRects dom/base/nsRange.cpp:3164 3 xul.dll static bool mozilla::dom::RangeBinding::getClientRects dom/bindings/RangeBinding.cpp:1486 4 xul.dll mozilla::dom::GenericBindingMethod dom/bindings/BindingUtils.cpp:3032 5 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:467 6 xul.dll js::ForwardingProxyHandler::call js/src/proxy/Wrapper.cpp:176 7 xul.dll js::InternalCallOrConstruct js/src/vm/Interpreter.cpp:449 8 xul.dll static bool js::jit::DoCallFallback js/src/jit/BaselineIC.cpp:2380 9 @0x16cf60e740e ============================================================= There are 8 crashes (from 6 installations) in nightly 61 with buildid 20180329222832. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1437509. [1] https://hg.mozilla.org/mozilla-central/rev?node=0a91126bb0f9
Flags: needinfo?(bwerth)
Attachment #8963974 - Flags: review?(bzbarsky)
Assignee: nobody → bwerth
Comment on attachment 8963974 [details] Bug 1450208: Change nsRange::ExtractRectFromOffset to use simpler, hopefully safer logic to determine whether text is vertical. https://reviewboard.mozilla.org/r/232790/#review238228 r=me though it would be good to have a description in the bug or commit message of what was wrong with the old code...
Attachment #8963974 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6) > Comment on attachment 8963974 [details] > r=me though it would be good to have a description in the bug or commit > message of what was wrong with the old code... I put a note in the commit message, stating that this patch avoids a static_cast that isn't necessary, and could fail under unusual conditions (when isTextFrame() lied).
When would IsTextFrame() lie?
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9) > When would IsTextFrame() lie? I don't know if it's even possible, though it's the best explanation I have for the crash signature. The version of the patch I just pushed has MOZ_ASSERT on both pointer parameters, and with the change to eliminate the static_cast, there's no way to do a bad dereference (in debug mode anyway). If nothing else, this will flush out what's causing this crash.
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/908749c3b858 Change nsRange::ExtractRectFromOffset to use simpler, hopefully safer logic to determine whether text is vertical. r=bz
https://hg.mozilla.org/mozilla-central/rev/908749c3b858
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Crash Signature: [@ static void ExtractRectFromOffset] → [@ static void ExtractRectFromOffset] [@ nsRange::CollectClientRectsAndText]
No more crashes since the patch landed.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
bug 1437509 got uplifted to 60.0b14 and this now seems to be a fairly frequent content crash in early stability data. so we probably need to uplift the patch here to beta too.
status-firefox60: unaffected → affected
tracking-firefox60: --- → ?
Crash Signature: [@ static void ExtractRectFromOffset] [@ nsRange::CollectClientRectsAndText] → [@ static void ExtractRectFromOffset] [@ nsRange::CollectClientRectsAndText] [@ ExtractRectFromOffset ]
Comment on attachment 8963974 [details] Bug 1450208: Change nsRange::ExtractRectFromOffset to use simpler, hopefully safer logic to determine whether text is vertical. Approval Request Comment [Feature/Bug causing the regression]: Bug 1437509 [User impact if declined]: Vertical text may not be findable [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: yes [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?]: Has baked in Nightly, avoids a static cast [String changes made/needed]: none
Attachment #8963974 - Flags: approval-mozilla-beta?
Comment on attachment 8963974 [details] Bug 1450208: Change nsRange::ExtractRectFromOffset to use simpler, hopefully safer logic to determine whether text is vertical. Regression from beta 14, let's uplift this for beta 15.
Attachment #8963974 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We should check on this in crash-stats next week after beta 15 is out, to verify.
Flags: qe-verify+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: