Closed Bug 613420 Opened 10 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

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

References

Details

Attachments

(1 file, 2 obsolete files)

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?
Attached patch patch (obsolete) — Splinter Review
Attachment #491758 - Flags: review?(roc)
Attachment #491758 - Flags: approval2.0?
Attachment #491758 - Flags: approval2.0? → approval2.0+
Assignee: nobody → ehren.m
Status: NEW → ASSIGNED
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/d5da40425a61
Status: ASSIGNED → RESOLVED
Closed: 10 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?
Attached 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.
Attached 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)
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]
http://hg.mozilla.org/mozilla-central/rev/a44bb02da741
Status: REOPENED → RESOLVED
Closed: 10 years ago9 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.