Closed
Bug 613420
Opened 14 years ago
Closed 13 years ago
nsFirstLetterFrame is a query frame target that does not declare |nsQueryFrame::FrameIID kFrameIID|
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla2.0b8
People
(Reporter: ehren.m, Assigned: ehren.m)
References
Details
Attachments
(1 file, 2 obsolete files)
1.56 KB,
patch
|
roc
:
review+
dbaron
:
approval2.0-
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Attachment #491758 -
Flags: review?(roc)
Attachment #491758 -
Flags: review?(roc) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #491758 -
Flags: approval2.0?
Updated•14 years ago
|
Attachment #491758 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Updated•14 years ago
|
Comment 2•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d5da40425a61
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Comment 3•14 years ago
|
||
The block of code should be executed by our crashtests at least.
Comment 4•14 years ago
|
||
backed out because of failing reftest/tests/layout/base/crashtests/436982-1.html
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 5•14 years ago
|
||
Actually, it won't let me push the backout right now for whatever reason...
Comment 6•14 years ago
|
||
Ok, pushed finally.
Comment 7•14 years ago
|
||
(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.
Comment 8•14 years ago
|
||
So is the crash because the patch doesn't have all the queryframe macros it needs?
Assignee | ||
Comment 9•14 years ago
|
||
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
Comment 10•14 years ago
|
||
We should probably just do queryframe correctly. I think you can follow the pattern of nsFrameSetFrame for that.
Assignee | ||
Comment 11•14 years ago
|
||
followed the pattern of nsHTMLFrameSetFrame.
Attachment #493483 -
Attachment is obsolete: true
Attachment #493527 -
Flags: review?(tnikkel)
Updated•14 years ago
|
Attachment #493527 -
Flags: review?(tnikkel) → review?(roc)
Attachment #493527 -
Flags: review?(roc) → review+
Assignee | ||
Updated•14 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-
Comment 13•13 years ago
|
||
Comment on attachment 491758 [details] [diff] [review] patch (bookkeeping)
Attachment #491758 -
Flags: approval2.0+
Updated•13 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
Comment 14•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a44bb02da741
Status: REOPENED → RESOLVED
Closed: 14 years ago → 13 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.
Description
•