Crash in static void ExtractRectFromOffset

VERIFIED FIXED in Firefox 60

Status

()

Product:
Toolkit
Component:
Find Toolbar
Type:
defect
Importance:
--
critical
Status:
VERIFIED FIXED
Opened:
Last year
Updated:
Last year

People

(Reporter: calixte, Assigned: bradwerth)

Tracking

(Blocks 1 bug, {crash, regression})

Version:
Trunk
Target:
mozilla61
Platform:
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Duplicates:
1455742

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60+ verified, firefox61 verified)

Details

(crash signature)

Attachments

(1 attachment)

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: Last year
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
Duplicate of this bug: 1455742
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+
You need to log in before you can comment on or make changes to this bug.