Closed Bug 80352 Opened 24 years ago Closed 20 years ago

Changing Bidi options never triggers reflow

Categories

(Core :: Internationalization, defect)

Product:
Core
Component:
Internationalization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: cathleennscp, Assigned: smontagu)

References

Details

(Keywords: fixed1.8.1)

Attachments

(4 files, 3 obsolete files)

test case trigger eReflowReason_StyleChange when the next page loaded
579 bytes, text/html
Details
temp fix untill simon@softel.co.il find out the real fix
803 bytes, patch
Details | Diff | Splinter Review
Patch for review
2.48 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Branch patch
5.44 KB, patch
bzbarsky
: approval-branch-1.8.1+
Details | Diff | Splinter Review
Marc Attinasi wrote: I'm getting the same results as Curtis, basically. Running the same source built with and without BiDi enabled I get the following reflow data Default Build with IBMBIDI defined: Document http://jrgm.mcom.com/perf/loadtime5/base/www.w3.org_DOML2Core loaded successfully Disabling View Source StyleSheet /perf/loadtime5/base/www.w3.org_DOML2Core/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 780 0 0 0 0 780 nsTableColGroupFrame 87 0 174 0 0 261 nsBoxToBlockAdaptor 1 28 0 0 0 29 nsGfxScrollFrame 1 26 0 0 0 27 nsTableOuterFrame 87 0 174 0 0 261 nsTextFrame 6226 0 9064 0 0 15290 nsTableRowFrame 110 0 220 0 0 330 nsInlineFrame 2752 0 3997 0 0 6749 ViewportFrame 1 26 0 0 0 27 CanvasFrame 1 26 2 0 0 29 nsTableCellFrame 330 0 435 0 0 765 BRFrame 79 0 66 0 0 145 nsBulletFrame 33 1 36 0 0 70 nsBlockFrame 1962 222 3597 0 0 5781 HRuleFrame 2 0 4 0 0 6 nsTableRowGroupFrame 87 0 174 0 0 261 nsBoxFrame 1 26 0 0 0 27 nsTableFrame 87 0 89 0 0 176 Grand Totals 12627 355 18032 0 0 31014 Build without IBMBIDI defined: Document http://jrgm.mcom.com/perf/loadtime5/base/www.w3.org_DOML2Core loaded successfully Disabling View Source StyleSheet /perf/loadtime5/base/www.w3.org_DOML2Core/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 549 0 0 0 0 549 nsTableColGroupFrame 87 0 97 0 0 184 nsBoxToBlockAdaptor 1 51 0 0 0 52 nsGfxScrollFrame 1 49 0 0 0 50 nsTableOuterFrame 87 0 21 0 0 108 nsTextFrame 6190 0 3129 0 0 9319 nsTableRowFrame 110 0 126 0 0 236 nsInlineFrame 2746 0 1531 0 0 4277 ViewportFrame 1 49 0 0 0 50 CanvasFrame 1 49 2 0 0 52 nsTableCellFrame 330 0 345 0 0 675 BRFrame 79 0 6 0 0 85 nsBulletFrame 33 0 28 0 0 61 nsBlockFrame 1962 394 1188 0 0 3544 HRuleFrame 2 0 2 0 0 4 nsTableRowGroupFrame 87 0 97 0 0 184 nsBoxFrame 1 49 0 0 0 50 nsTableFrame 87 0 12 0 0 99 Grand Totals 12354 641 6584 0 0 19579 I'd like to start narrowing down the IBMBIDI sections that are causing this, since I cannot see it from a code inspection. nsTextFrame and nsBlockFrame are my initial candidates, and I'll start disabling selectively the IBMBIDI sections from those files an rerunning. - marc
OS: Windows 2000 → All
Hardware: PC → All
I just call marc and he said the number he post is wrong. He somehow build it from his branch and both number are without bidi IT is interesting that these two number are so different in the same build.
Yup - my really really bad. In narrowing this down I found out that I actually DID NOT have IBMBIDI defined in either of my builds, so the numbers I posted are actually for the same build! It is interesting that the numbers are so different, and to be honest I ran this build about 5-6 times and the numbers were different each time - I do not know why (timing maybe?) So, please ignore the numbers posted when the bug was opened. I am building again to redo the analysis I tried to do before (sigh).
Also, I look with Curt with the raw data file (for example ftp://ftp.mozilla.org/pub/data/reflows/010510/reflow.out ) I think the tool he used to add up the number from the raw data file (reflow.out) have some problem-. It basically use "+++ loading" as the seperate between URL. But if you read the reflow.out file yourself, you will find out that there are different urls load between "+++ loading" . So the number we currently got are not accurate for a particulare URL. Regardless of the above issue, the BIDI one is still bigger. In particular, if we read the reflow.out carefully, the non bidi one typically have Style and Dirty one in 0 for every one of it. But the Bidi one have large number in the Style and some non zero number for Dirty. I think that is the reason the reflow count went up. I talk to marc about this and we are looking why the Style one went up now.
I know some cases where Bidi processing can happen on non-Bidi pages. I've added an assertion in my local tree to detect this and was going to open a bug on all the cases I have found. On Sunday I'll investigate reflow with and without my local changes.
somehow, the result.out in the ftp.mozilla.org show some number in the Style and Dirty reflow. But in my Window build with Viewer, I always get 0 in Style and Dirty for these pages. Maybe we should also read the Linux warnning to see any stuff we need to think about. simon@softel.co.il probably know this best. It is hard for him because I cannot see this on window and he is on window.
Status: NEW → ASSIGNED
I run the bidi build on linux with viewer and the dump does not have number for Style and Dirty. I wonder why gtkEmbed see that What is --eable-cpp-rtti do ? Will that make some differences?
Bidi resolution may cause additional reflow (nsBlockFrame.cpp, around line 711). Main conditions: 1. Bidi enabled (as a result of presense of hebrew/arabic characters or/and dir=rtl attribute). (BTW this means that "pure Latin" frames wouldn't be involved.) 2. At least 1 frame was removed during bidi resolution.
Lina- I look at those pages. I don't think those pages fall into your condiction. According to the raw data (see ftp://ftp.mozilla.org/pub/data/reflows/010510/reflow.out ) I see a lot of Style reflow which does not happen before bidi land . Also check Dirty reflow
hum.... I don't think the issue is window or linux. I have the up to date linux build. And I cannot see the problem in viewer. Howerver, if I run the reflow test with gtkEmbed. I can see the Style reflow count is high. This is strange. For example- let's lookat at the loading of http://jrgm.mcom.com/perf/loadtime5/ base/www.apple.com in ftp://ftp.mozilla.org/pub/data/reflows/010508/reflow.out /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 49 0 0 0 0 49 nsTableColGroupFrame 11 0 21 0 0 32 nsBoxToBlockAdaptor 2 7 0 0 0 9 nsComboboxControlFrame 1 0 2 0 0 3 nsTableOuterFrame 11 2 20 0 0 33 nsGfxScrollFrame 1 5 0 0 0 6 nsTextFrame 136 0 137 0 0 273 nsListControlFrame 1 0 1 0 0 2 nsTableRowFrame 16 2 29 0 0 47 nsInlineFrame 45 0 21 0 0 66 nsPlaceholderFrame 2 0 2 0 0 4 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 0 0 15 CanvasFrame 1 5 2 0 0 8 nsHTMLButtonControlFrame 1 0 0 0 0 1 nsTableCellFrame 22 2 22 0 0 46 BRFrame 13 0 15 0 0 28 nsBlockFrame 79 15 80 0 0 174 nsTableRowGroupFrame 11 2 21 0 0 34 nsGfxTextControlFrame2 1 0 1 0 0 2 nsImageFrame 36 0 11 0 1 48 nsBoxFrame 2 5 1 0 0 8 nsTableFrame 11 2 11 0 0 24 nsScrollFrame 1 0 1 0 0 2 Grand Totals 462 52 405 0 1 920 in ftp://ftp.mozilla.org/pub/data/reflows/010510/reflow.out /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 66 0 0 0 0 66 nsTableColGroupFrame 11 0 32 0 0 43 nsBoxToBlockAdaptor 2 7 0 2 0 11 nsComboboxControlFrame 1 0 2 1 0 4 nsTableOuterFrame 11 1 20 11 0 43 nsGfxScrollFrame 1 5 0 0 0 6 nsTextFrame 136 0 129 168 0 433 nsListControlFrame 1 0 1 2 0 4 nsTableRowFrame 16 1 45 16 0 78 nsInlineFrame 45 0 21 81 0 147 nsPlaceholderFrame 2 0 2 4 0 8 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 15 0 30 nsHTMLButtonControlFrame 1 0 0 1 0 2 CanvasFrame 1 4 2 1 0 8 nsTableCellFrame 22 1 22 22 0 67 BRFrame 13 0 15 18 0 46 nsBlockFrame 79 10 125 81 0 295 nsTableRowGroupFrame 11 1 32 11 0 55 nsGfxTextControlFrame2 1 0 1 2 0 4 nsImageFrame 36 0 11 71 1 119 nsBoxFrame 2 5 1 2 0 10 nsTableFrame 11 1 11 11 0 34 nsScrollFrame 1 0 2 1 0 4 Grand Totals 479 41 481 521 1 1523 and in my local build which run the gtkEmbed- /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 66 0 0 0 0 66 nsTableColGroupFrame 11 0 32 0 0 43 nsBoxToBlockAdaptor 2 7 0 2 0 11 nsComboboxControlFrame 1 0 2 1 0 4 nsTableOuterFrame 11 1 20 11 0 43 nsGfxScrollFrame 1 5 0 0 0 6 nsTextFrame 136 0 132 168 0 436 nsListControlFrame 1 0 1 2 0 4 nsTableRowFrame 16 1 45 16 0 78 nsInlineFrame 45 0 21 81 0 147 nsPlaceholderFrame 2 0 2 4 0 8 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 15 0 30 nsHTMLButtonControlFrame 1 0 0 1 0 2 CanvasFrame 1 4 2 1 0 8 nsTableCellFrame 22 1 22 22 0 67 BRFrame 13 0 15 18 0 46 nsBlockFrame 79 11 125 81 0 296 nsTableRowGroupFrame 11 1 32 11 0 55 nsGfxTextControlFrame2 1 0 1 2 0 4 nsImageFrame 36 0 11 71 1 119 nsBoxFrame 2 5 1 2 0 10 nsTableFrame 11 1 11 11 0 34 nsScrollFrame 1 0 2 1 0 4 Grand Totals 479 42 484 521 1 1527 which you can also see there are non-zero Style reflow. However, in the same linux build, if I run viewer and dump the reflow test by hand, I got the following result /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 49 0 0 0 0 49 nsTableColGroupFrame 11 0 21 0 0 32 nsBoxToBlockAdaptor 2 5 0 0 0 7 nsComboboxControlFrame 1 0 2 0 0 3 nsGfxScrollFrame 1 5 0 0 0 6 nsTableOuterFrame 11 1 20 0 0 32 nsTextFrame 136 0 129 0 0 265 nsListControlFrame 1 0 1 0 0 2 nsTableRowFrame 16 1 29 0 0 46 nsInlineFrame 45 0 21 0 0 66 nsPlaceholderFrame 2 0 2 0 0 4 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 0 0 15 CanvasFrame 1 3 2 0 0 6 nsHTMLButtonControlFrame 1 0 0 0 0 1 nsTableCellFrame 22 1 22 0 0 45 BRFrame 13 0 15 0 0 28 nsBlockFrame 79 8 80 0 1 168 nsTableRowGroupFrame 11 1 21 0 0 33 nsGfxTextControlFrame2 1 0 1 0 0 2 nsBoxFrame 2 5 1 0 0 8 nsImageFrame 36 0 11 0 1 48 nsTableFrame 11 1 11 0 0 23 nsScrollFrame 1 0 1 0 0 2 Grand Totals 462 36 397 0 2 897 Notice that there are no Style reflow and the total total reflow number is smaller than no bidi
Interesting, I modify test/launch_reflow_test.csh and change setenv URL_COUNT 64 ./watch_reflow.sh -o $OUTFILE -u $URL_COUNT $MOZILLA_FIVE_HOME/gtkEmbed "http:// intrepid/buster.cgi?interval=10&file=lists/reflowX46_ltog.list" >>& $OUTFILE to setenv URL_COUNT 1 ./watch_reflow.sh -o $OUTFILE -u $URL_COUNT $MOZILLA_FIVE_HOME/gtkEmbed "http:// jrgm.mcom.com/perf/loadtime5/base/www.apple.com" >>& $OUTFILE and I got the following result /perf/loadtime5/base/www.apple.com/ Init Incrm Resze Style Dirty Total ------------------------------------------------------------------------------ nsTableColFrame 49 0 0 0 0 49 nsTableColGroupFrame 11 0 21 0 0 32 nsBoxToBlockAdaptor 2 5 0 0 0 7 nsComboboxControlFrame 1 0 2 0 0 3 nsGfxScrollFrame 1 5 0 0 0 6 nsTableOuterFrame 11 1 20 0 0 32 nsTextFrame 136 0 129 0 0 265 nsListControlFrame 1 0 1 0 0 2 nsTableRowFrame 16 1 29 0 0 46 nsInlineFrame 45 0 21 0 0 66 nsPlaceholderFrame 2 0 2 0 0 4 ViewportFrame 1 5 0 0 0 6 nsGfxButtonControlFrame 8 0 7 0 0 15 CanvasFrame 1 3 2 0 0 6 nsHTMLButtonControlFrame 1 0 0 0 0 1 nsTableCellFrame 22 1 22 0 0 45 BRFrame 13 0 15 0 0 28 nsBlockFrame 79 8 80 0 1 168 nsTableRowGroupFrame 11 1 21 0 0 33 nsGfxTextControlFrame2 1 0 1 0 0 2 nsBoxFrame 2 5 1 0 0 8 nsImageFrame 36 0 11 0 1 48 nsTableFrame 11 1 11 0 0 23 nsScrollFrame 1 0 1 0 0 2 Grand Totals 462 36 397 0 2 897 This mean the Style reflow is not increase if we simply load http:// jrgm.mcom.com/perf/loadtime5/base/www.apple.com . It is casued by loading http:// intrepid/buster.cgi?interval=10&file=lists/reflowX46_ltog.list . We should look at how these test script interact with frame.
here is the source of http://intrepid/buster.cgi?interval=10&file=lists/ reflowX46_ltog.list <html> <head> <meta http-equiv="Pragma" content="no-cache"> <meta http-equiv="refresh" content="30;url=buster.cgi?file=lists/ reflowX46_ltog.list&page=1&last=-1&refresh=30"> <title>BrowserBuster II: http://jrgm.mcom.com/perf/loadtime5/base/www.google.com< /title> <style type="text/css"> body { overflow: hidden; border: 0; margin: 0; } </style> </head> <script> dump("+++ loading http://jrgm.mcom.com/perf/loadtime5/base/www.google.com\n"); </script> <body> lists/reflowX46_ltog.list: http://jrgm.mcom.com/perf/loadtime5/base/ www.google.com <iframe width="100%" height="100%" src="http://jrgm.mcom.com/perf/loadtime5/base/ www.google.com"> </body> </html> notice body { overflow: hidden; border: 0; margin: 0; } and <iframe width="100%" height="100%" What are these stuff impact to the Style reflow ? I think the test procedure have some impact to the test result
I should try this url
The test is very strang- it will first load the apple.com and with a reasonable reflow number, and when it try to load the next sites, it will first dump a loading of the new sites and then dump a bigger number of the last sites which it load, the reason the number is bigger is because style reflow. The bidi reflow does increase in the 2nd number in the style reflow. But please notice that this style reflow increase won't be reproduceable if just load the web site by hand, it only could be reproduced if combined with iframe and some style sheet stuff. I wonder what is overflow: hidden; mean
ok. the body { overflow: hidden; border: 0; margin: 0; } have nothing to do with the issue. Also, the height and width of the iframe have nothing to do with this issue.
I made a simpiler test cases- 1. open viewer and view that test case 2. type in another url- say an empty html the style change happen when we load the empty html on the test page
reproduce procedure 1. make a html file called a.html which only contains "hello" 2. on window (or linux), set your debugging application to viewer 3. set a break point at nsTextFrame::Reflow and make the condiction as aReflowState.reason == eReflowReason_StyleChange 4. run viewer 5. load http://bugzilla.mozilla.org/showattachment.cgi?attach_id=34363 into viewer 6. no break happen yet 7. view a.html which you create at 1 it will break at nsTextFrame::Reflow, but what got Style change is not a.html bu thte test page. I also set break point at all the place which I can find " = eReflowReason_StyleChange" and the earilest break point I hit is the following layout/xul/base/src/nsBoxToBlockAdaptor.cpp#826 819 // if we were marked for style change. 820 // 1) see if we are just supposed to do a resize if so convert to a style change. Kill 2 birds 821 // with 1 stone. 822 // 2) If the command is incremental. See if its style change. If it is everything is ok if not 823 // we need to do a second reflow with the style change. 824 if (mStyleChange) { 825 if (reflowState.reason == eReflowReason_Resize) 826 reflowState.reason = eReflowReason_StyleChange; 827 else if (reason == eReflowReason_Incremental) { and here is the stack trace- nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000, int 0x00000000, int 0x00000000, int 0x000021de, int 0x00001077, int 0x00000001) line 826 nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x01a00db4, nsBoxLayoutState & {...}) line 523 + 52 bytes nsBox::Layout(nsBox * const 0x01a00db4, nsBoxLayoutState & {...}) line 985 nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x01a001d4, nsBoxLayoutState & {...}) line 377 nsBox::Layout(nsBox * const 0x01a001d4, nsBoxLayoutState & {...}) line 985 nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x01a001d4, const nsRect & {...}) line 591 + 16 bytes nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x01a001d4, const nsRect & {...}) line 1038 + 17 bytes nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1144 nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x01a0012c, nsBoxLayoutState & {...}) line 1046 + 15 bytes nsBox::Layout(nsBox * const 0x01a0012c, nsBoxLayoutState & {...}) line 985 nsBoxFrame::Reflow(nsBoxFrame * const 0x01a000f4, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 781 nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x01a000f4, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 735 + 25 bytes nsContainerFrame::ReflowChild(nsIFrame * 0x01a000f4, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 0x00000000, int 0x00000000, unsigned int 0x00000000, unsigned int & 0x00000000) line 722 + 31 bytes ViewportFrame::Reflow(ViewportFrame * const 0x01a00080, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 538 nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x037338e0, nsIPresContext * 0x029c65a0, nsHTMLReflowMetrics & {...}, const nsSize & {...}, nsIRenderingContext & {...}) line 145 PresShell::ProcessReflowCommand(nsVoidArray & {...}, int 0x00000001, nsHTMLReflowMetrics & {...}, nsSize & {...}, nsIRenderingContext & {...}) line 5761 PresShell::ProcessReflowCommands(int 0x00000001) line 5816 ReflowEvent::HandleEvent() line 5674 HandlePLEvent(ReflowEvent * 0x03733880) line 5688 PL_HandleEvent(PLEvent * 0x03733880) line 588 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x0106aae0) line 518 + 9 bytes _md_EventReceiverProc(HWND__ * 0x11b0074c, unsigned int 0x0000c14e, unsigned int 0x00000000, long 0x0106aae0) line 1069 + 9 bytes USER32! 77e41186() 0106aae0() It looks like the iframe is wrap inside the nsBox and we try to resize the Box when we load the new page The aDesiredSize is 0 and 0 in PresShell::ProcessReflowCommand
Ok, here is the reason why we got the reflow in the bidi build nsHTMLReflowCommand::nsHTMLReflowCommand(nsIFrame * 0x01b6414c, nsIReflowCommand::ReflowType ReflowDirty, nsIFrame * 0x00000000, nsIAtom * 0x00000000) line 67 NS_NewHTMLReflowCommand(nsIReflowCommand * * 0x0012f814, nsIFrame * 0x01b6414c, nsIReflowCommand::ReflowType ReflowDirty, nsIFrame * 0x00000000, nsIAtom * 0x00000000) line 47 + 43 bytes nsFrame::CreateAndPostReflowCommand(nsIPresShell * 0x0331dd30, nsIFrame * 0x01b6414c, nsIReflowCommand::ReflowType ReflowDirty, nsIFrame * 0x00000000, nsIAtom * 0x00000000, nsIAtom * 0x00000000) line 3923 + 45 bytes nsContainerFrame::ReflowDirtyChild(nsContainerFrame * const 0x01b640d8, nsIPresShell * 0x0331dd30, nsIFrame * 0x01b6414c) line 332 + 21 bytes nsBox::MarkStyleChange(nsBox * const 0x01b64184, nsBoxLayoutState & {...}) line 327 + 28 bytes nsPresContext::RemapStyleAndReflow(nsPresContext * const 0x03310750) line 441 nsPresContext::SetBidi(nsPresContext * const 0x03310750, unsigned int 0x00000000, int 0x00000001) line 1536 DocumentViewerImpl::SetBidiOptions(DocumentViewerImpl * const 0x032e577c, unsigned int 0x00000000) line 4926 SetChildBidiOptions(nsIMarkupDocumentViewer * 0x032e577c, void * 0x00000000) line 4740 DocumentViewerImpl::CallChildren(void (nsIMarkupDocumentViewer *, void *)* 0x016b4e80 SetChildBidiOptions(nsIMarkupDocumentViewer *, void *), void * 0x00000000) line 4560 + 16 bytes DocumentViewerImpl::SetBidiOptions(DocumentViewerImpl * const 0x03398c3c, unsigned int 0x00000000) line 4928 nsDocShell::SetupNewViewer(nsDocShell * const 0x012cec80, nsIContentViewer * 0x03398c30) line 3278 + 19 bytes nsWebShell::SetupNewViewer(nsWebShell * const 0x012cec80, nsIContentViewer * 0x03398c30) line 346 + 13 bytes nsDocShell::Embed(nsDocShell * const 0x012ceca0, nsIContentViewer * 0x03398c30, const char * 0x01604610, nsISupports * 0x00000000) line 2764 + 23 bytes nsWebShell::Embed(nsWebShell * const 0x012ceca0, nsIContentViewer * 0x03398c30, const char * 0x01604610, nsISupports * 0x00000000) line 375 nsDocShell::CreateContentViewer(nsDocShell * const 0x012cec80, const char * 0x0012fd7c, nsIRequest * 0x02e02d80, nsIStreamListener * * 0x0012fdcc) line 3043 + 32 bytes nsDSURIContentListener::DoContent(nsDSURIContentListener * const 0x012ce970, const char * 0x0012fd7c, int 0x00000000, const char * 0x002e8220 gCommonEmptyBuffer, nsIRequest * 0x02e02d80, nsIStreamListener * * 0x0012fdcc, int * 0x0012fd60) line 117 + 33 bytes nsDocumentOpenInfo::DispatchContent(nsIRequest * 0x02e02d80, nsISupports * 0x00000000) line 371 + 109 bytes nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x02e04500, nsIRequest * 0x02e02d80, nsISupports * 0x00000000) line 240 + 16 bytes nsResChannel::OnStartRequest(nsResChannel * const 0x02e02d88, nsIRequest * 0x02e041a0, nsISupports * 0x00000000) line 532 nsFileChannel::OnStartRequest(nsFileChannel * const 0x02e041a8, nsIRequest * 0x02e055a4, nsISupports * 0x00000000) line 453 + 37 bytes nsOnStartRequestEvent::HandleEvent() line 108 + 53 bytes nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x02e05b24) line 64 PL_HandleEvent(PLEvent * 0x02e05b24) line 588 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x012ca0e0) line 518 + 9 bytes _md_EventReceiverProc(HWND__ * 0x1de10236, unsigned int 0x0000c14e, unsigned int 0x00000000, long 0x012ca0e0) line 1069 + 9 bytes USER32! 77e41186() 012ca0e0()
To get the previous stack trace, 1. run viewer, 2. load resource:///res/samples/test9.html 3. set break point at nsHTMLReflowCommand::nsHTMLReflowCommand and 4. load resource:///res/samples/test0.html it will break here Notice that you have to use test9.html because it have frame
OK, here is the problem code nsPresContext::SetBidi .... if (mShell && aForceReflow) { RemapStyleAndReflow(); } and DocumentViewerImpl::SetBidiOptions if (mPresContext) { mPresContext->SetBidi(aBidiOptions, PR_TRUE); // force reflow } DocumentViewerImpl::SetBidiOptions always pass in PR_TRUE into nsPresContext::SetBidi which cause it call RemapStyleAndReflow();
jst- can you r/sr my temp fix ?
sr=vidur for the temporary disabling of on-the-fly bidi change. The fix to figure out why extra reflows are happening (presumably for the new page) should be investigated ASAP.
r=jst
Status: ASSIGNED → NEW
temp fix check in. Down grade the bug from moz0.9.1 reassign this bug to simon@softel.co.il
Assignee: ftang → simon
Have a fix for this, but it's hard to separate the diffs from those for bug 80552. When that gets checked in I'll attach a proper patch. Meanwhile, here is the new method. The idea is to remove the aForceReflow argument from nsPresContext::SetBidi and let the method itself decide whether the bidi options require a reflow. NS_IMETHODIMP nsPresContext::SetBidi(PRUint32 aSource) { PRBool isVisual, bidiEnabled; PRBool needReflow = PR_FALSE; // Test for changes to Bidi options that require reflow // 1. Change in page direction: if (GET_BIDI_OPTION_DIRECTION(mBidi) != GET_BIDI_OPTION_DIRECTION(aSource)) needReflow = PR_TRUE; // 2. Change in text type (logical/visual) or bidi enabled status. // Not enough just to test for a change to the options, because these // depend on combinations of different options. So save the previous // state and reflow if there is any change. isVisual = mIsVisual; GetBidiEnabled(&bidiEnabled); mBidi = aSource; if (IBMBIDI_TEXTDIRECTION_RTL == GET_BIDI_OPTION_DIRECTION(mBidi) || IBMBIDI_NUMERAL_HINDI == GET_BIDI_OPTION_NUMERAL(mBidi)) { if (!bidiEnabled) { needReflow = PR_TRUE; } SetBidiEnabled(PR_TRUE); } if (IBMBIDI_TEXTTYPE_VISUAL == GET_BIDI_OPTION_TEXTTYPE(mBidi)) { SetVisualMode(PR_TRUE); } else if (IBMBIDI_TEXTTYPE_LOGICAL == GET_BIDI_OPTION_TEXTTYPE(mBidi)) { SetVisualMode(PR_FALSE); } else { SetVisualMode(IsVisualCharset(mCharset) ); } if (isVisual != mIsVisual) { needReflow = PR_TRUE; } if (needReflow) { // Finally check for bidiEnabled and only reflow if it is set GetBidiEnabled(&bidiEnabled); if (bidiEnabled) { RemapStyleAndReflow(); } } return NS_OK; }
Whiteboard: have fix. need to produce patch
80552 have been reviewed and checked in . Please update your tree and propose a real fix for this bug :) Thanks
Attached patch the real fix (obsolete) — — Splinter Review
for long comments would you consider /* */ or /** */ notations?
OK, I'll consider it. :-) Seriously, commenting notation doesn't seem to be mentioned in any of the style and convention docs on mozilla.org (except for http://www.mozilla.org/hacking/portable-cpp.html#no_cpp_comments_in_c) What many of the docs do emphasize is consistency within a given source file, and nsPresContext.cpp uses // almost without exception.
simon- what will happen if we don't take your real fix for moz0.9.1 ? what does that mean. Could we wait till moz0.9.2 ?
I will try it when I have time.
I think the real fix will only make a difference if bidi prefs are enabled, so yes, it can wait until 0.9.2
should we consider check into trunk now?
Marking as dependent on bug 88100. The patch here will need some revision once that one is checked in.
Depends on: 88100
Blocks: 115707
No longer blocks: 115707
Blocks: 115715
QA Contact: andreasb → zach
Status: NEW → ASSIGNED
Comment 35 is not quite right: we need to force reflow also when changing properties from javascript. I'll bring the patch up to date and check in soon.
We should probably actually fix this...
Yes, but changing summary to make it clear what we are fixing. The increased reflow count was fixed by attachment 34391 [details] [diff] [review]. Asaf, do you know any case where dynamically changing Bidi options doesn't cause the expected change in the rendering?
Summary: reflow count increased on 5/10 build, due to BiDi code? → Changing Bidi options never triggers reflow
Attached patch Updated patch (obsolete) — — Splinter Review
There are really two parts to this. The first part, as described in comment 28 SetBidi() determines internally whether a reflow is necessary by comparing the old and new options. The second part is intended to prevent the temporary fix from regressing: if (0 == mBidi) { // if mBidi is zero, this is a new PresContext so no need to reflow // (bug 80352) needReflow = PR_FALSE; } I rather suspect that this test is bogus.
Attachment #34931 - Attachment is obsolete: true
Comment on attachment 202265 [details] [diff] [review] Updated patch Oh, hum. I didn't realize you were touching this code in another bug, Boris. I should read my bugmail more synchronously :(
Attachment #202265 - Attachment is obsolete: true
Sorry... I was considering commenting on this bug and pointing to that one, but figured you were cced on it... :(
Attached patch Updated and unbitrotted patch (obsolete) — — Splinter Review
I decided that the tests in the original patch for whether reflow was required were over-optimization: a script might set one option after the other, and each individual change would not seem to require a reflow, but the accumulation of all of them would. So I am just testing whether any bits change, plus the equivalent of the |if (0 == mBidi)| test in the previous patch.
Some testcases: Go to a logical Bidi page, e.g. http://unicode.org/iuc/iuc10/x-ar.html Open a new tab and use about:config to change bidi.texttype to 3 Return to the previous tab. Expected results: the numbers within the Arabic text should be reversed: 21-01 and 7991 instead of 10-12 and 1997. Actual results: the numbers remain the same unless the page is reloaded. Go to a visual Bidi page, e.g. http://www.cs.bgu.ac.il/~elad/noa_v.html Open a new tab and use about:config to change bidi.texttype to 2 Return to the previous tab. Expected results: all the Hebrew text should be reversed: העונ instead of נועה Actual results: the text remains the same unless the page is reloaded.
Attached patch Patch for review — — Splinter Review
Some slight changes here: testing for the current bidi options being zero seems not to be so bogus, since they are set to zero by nsDocument::operator new(). However, given that nsPresContext::GetUserPreferences() is calling SetBidi() without aForceReflow, and that since the fix of bug 88100 DocumentViewerImpl::SetBidiOptions() is no longer called from nsDocShell::SetupNewViewer as in the stack in comment 20, there seems to be no case where the scenario that caused the original bug report will occur, so I have changed that test to an assertion.
Attachment #202389 - Attachment is obsolete: true
Attachment #202512 - Flags: superreview?
Attachment #202512 - Flags: review?
Attachment #202512 - Flags: superreview?(bzbarsky)
Attachment #202512 - Flags: superreview?
Attachment #202512 - Flags: review?(bzbarsky)
Attachment #202512 - Flags: review?
Attachment #202512 - Flags: superreview?(bzbarsky)
Attachment #202512 - Flags: superreview+
Attachment #202512 - Flags: review?(bzbarsky)
Attachment #202512 - Flags: review+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Flags: blocking1.8.1?
1.8.1 blocker, but please try to get a 1.8-friendly patch soon.
Flags: blocking1.8.1? → blocking1.8.1+
Attached patch Branch patch — — Splinter Review
Attachment #219501 - Flags: approval-branch-1.8.1?(bzbarsky)
Attachment #219501 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1+
Checked into MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Whiteboard: have fix. need to produce patch
Blocks: 338103
No longer blocks: 338103
Depends on: 338103
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: