nsFirstLetterFrame is a query frame target that does not declare |nsQueryFrame::FrameIID kFrameIID|

RESOLVED FIXED in mozilla2.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: ehren.m, Assigned: ehren.m)

Tracking

unspecified
mozilla2.0b8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

9 years ago
This was discovered by our static-checking suite (specifically layout/generic/frame-verify.js).

Seems like http://hg.mozilla.org/mozilla-central/file/2e35085ca7cc/layout/base/nsBidiPresUtils.cpp#l212 is rarely executed if ever?
Assignee

Comment 1

9 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #491758 - Flags: review?(roc)
Assignee

Updated

9 years ago
Attachment #491758 - Flags: approval2.0?

Updated

9 years ago
Attachment #491758 - Flags: approval2.0? → approval2.0+
Assignee

Updated

9 years ago
Assignee: nobody → ehren.m
Status: NEW → ASSIGNED
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/d5da40425a61
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
The block of code should be executed by our crashtests at least.
backed out because of failing reftest/tests/layout/base/crashtests/436982-1.html
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Actually, it won't let me push the backout right now for whatever reason...
Ok, pushed finally.
(In reply to comment #0)
> Seems like
> http://hg.mozilla.org/mozilla-central/file/2e35085ca7cc/layout/base/nsBidiPresUtils.cpp#l212
> is rarely executed if ever?

More likely the GetType above means that it ends up doing the "right" thing.
So is the crash because the patch doesn't have all the queryframe macros it needs?
Assignee

Comment 9

9 years ago
Posted patch patch v2 (obsolete) — Splinter Review
I think this should solve the issue. Patch v1 is definitely incorrect and results in nsnull returned by nsQueryFrame (it eventually defaults to NS_QUERYFRAME_TAIL_INHERITANCE_ROOT). Without the patch the behavior is correct but only because nsQueryFrame eventually defaults to NS_QUERYFRAME_TAIL_INHERITING (I'd have to look at the pre-processed source to really understand this though).

In any case, replacing the do_QueryFrame call with a static_cast gives me the same behavior as the unpatched code when stepping through the subsequent call to CreateContinuationForFloatingParent (a normal cast is basically what QueryFrame ends up doing anyway).

I've got a try server and static analysis build going now though so I'll wait till they complete before requesting review (should have done this the first time!).
Attachment #491758 - Attachment is obsolete: true
We should probably just do queryframe correctly. I think you can follow the pattern of nsFrameSetFrame for that.
Assignee

Comment 11

9 years ago
Posted patch patch v3Splinter Review
followed the pattern of nsHTMLFrameSetFrame.
Attachment #493483 - Attachment is obsolete: true
Attachment #493527 - Flags: review?(tnikkel)
Attachment #493527 - Flags: review?(tnikkel) → review?(roc)
Assignee

Updated

9 years ago
Attachment #493527 - Flags: approval2.0?
Comment on attachment 493527 [details] [diff] [review]
patch v3

I think we should hold off on this and land it after we branch for Firefox 4 / Gecko 2.
Attachment #493527 - Flags: approval2.0? → approval2.0-
Depends on: post2.0
Keywords: checkin-needed
Whiteboard: [needs landing]

Comment 14

8 years ago
http://hg.mozilla.org/mozilla-central/rev/a44bb02da741
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.