Closed
Bug 793233
Opened 12 years ago
Closed 12 years ago
bidi reftest 779003, 712600 broken with dynamic changes
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
VERIFIED
FIXED
mozilla19
People
(Reporter: vlad, Assigned: smontagu)
References
Details
Attachments
(6 files)
1.08 KB,
text/html
|
Details | |
553 bytes,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
2.33 KB,
text/html
|
Details | |
2.15 KB,
text/html
|
Details | |
1.27 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
5.65 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
I'm actually hitting this with the patch in bug 731974 that changes the way nsRefreshDriver's timers work, but I discovered that the "wrong" rendering also happens on nightly if the reftest content is dynamically changed in a trivial way. The attached testcase is a slightly modified version of the reftest, that just removes a trivial <li> node a second after the page loads. The rendering completely changes in a nightly build, and ends up looks like the "broken" case after my patch.
This is interesting: I'm used to seeing dynamic invalidation bugs where forcing a reflow or forcing a frame reconstruct makes the bug go away, and then once it's gone it stays gone.
This one is unusual in that forcing a reflow and forcing a frame reconstruct *both* cause a state change if the most recent change was the other one. In particular, if I open Web console, and alternately do:
document.body.style.fontSize='30px';getComputedStyle(document.body, "").width;document.body.style.fontSize=''
document.body.style.display='none';getComputedStyle(document.body, "").width;document.body.style.display=''
I end up switching between the two states.
Reporter | ||
Comment 2•12 years ago
|
||
David, Simon, are either/both of you ok with marking this reftest as random for now?
Attachment #665529 -
Flags: review?(smontagu)
Reporter | ||
Comment 3•12 years ago
|
||
Hrm. Other tests fail in this same way -- dynamic version of 712600-1.
Reporter | ||
Comment 4•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
status-firefox18:
--- → affected
tracking-firefox18:
--- → ?
Summary: reftest 779003-1.html broken with dynamic changes → bidi reftest 779003, 712600 broken with dynamic changes
Comment 5•12 years ago
|
||
Did you mean to set this as tracking?
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 665529 [details] [diff] [review]
mark this reftest as random
Review of attachment 665529 [details] [diff] [review]:
-----------------------------------------------------------------
OK, mark the tests random for now, then leave the bug open and assign it to me for further investigation.
Attachment #665529 -
Flags: review?(smontagu) → review+
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a62f61bf19a0
Please add [leave open] to the whiteboard when you want a bug left open so our merge tools don't auto-resolve it.
Assignee: nobody → vladimir
Whiteboard: [leave open]
Updated•12 years ago
|
Assignee: vladimir → smontagu
Reporter | ||
Comment 8•12 years ago
|
||
Whoops, I didn't actually press Save Changes when I did all that to the bug.. sorry!
Assignee: smontagu → bugs
Reporter | ||
Updated•12 years ago
|
Assignee: bugs → smontagu
Comment 9•12 years ago
|
||
Clearing the tracking-firefox18 flag, as I think it was set by mistake ?!?! Please renominate if it needs to be tracked .
status-firefox18:
affected → ---
Reporter | ||
Comment 10•12 years ago
|
||
Hm, not sure why it looked like a mistake? It's an issue that affects fx18 and is potentially a regression; it might be tracking- if we don't care, but it was nominated on purpose...
status-firefox18:
--- → affected
Comment 11•12 years ago
|
||
Hey Vlad, apologies for not being clear on my comment 9 .
Reframing it here & leaving it open for nomination :
It will be very helpful if we can have the user impact outlined to track it for release . Was not sure if the rendering impact affected our users visibly or was it only the tests ?
Reporter | ||
Comment 12•12 years ago
|
||
Simon may be able to describe user impact better, but it seems like it could be an issue that would affect any sites doing RTL and dynamic html manipulation.. they would end up with fairly broken rendering.
Comment 13•12 years ago
|
||
Simon, can you please help with some input on comment 12 ?It will give us better idea to decide if we need to track it for release. Thanks !
Also do we know of any URL's where the impact can be seen ?
Assignee | ||
Comment 14•12 years ago
|
||
I don't know for sure if this has user-facing impact, but similar bugs (e.g. bug 731594) have caused backward text on Facebook, so I would rather play safe and track this for release.
Updated•12 years ago
|
Assignee | ||
Comment 15•12 years ago
|
||
The bug here even has a comment justifying it, but I didn't think it through properly: it's true that SplitInlineAncestors doesn't blow up with a null parameter, but in this context it doesn't make sense to call it if we don't have a previous sibling: the point of this code is to split runs of text with bidi isolation in separate frames, so if there is no previous sibling there is nothing to split, and we end up creating an empty inline. Then since the inline has no text frame containees with embeddingLevel property, reordering gets messed up.
Attachment #675005 -
Flags: review?(roc)
Assignee | ||
Comment 16•12 years ago
|
||
This doesn't include the dynamic version of 712600-1, since AFAICT the difference in rendering after deleting LTR text at the beginning of an RTL paragraph is expected.
Attachment #675006 -
Flags: review?(roc)
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #675005 -
Flags: review?(roc) → review+
Attachment #675006 -
Flags: review?(roc) → review+
Assignee | ||
Comment 18•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b339983c1dd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9563705dac
Flags: in-testsuite+
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [leave open]
Comment 19•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b339983c1dd5
https://hg.mozilla.org/mozilla-central/rev/8a9563705dac
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 20•12 years ago
|
||
Saw the issue on 19.0a1 (2012-10-24).
Verified fixed on 19.0a1 (2012-10-28) on Win 7, Ubuntu 12.04 and Mac OS X 10.7.5
Status: RESOLVED → VERIFIED
Comment 21•12 years ago
|
||
(In reply to Simon Montagu from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b339983c1dd5
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9563705dac
Please nominate for aurora uplift after enough bake time along with risk assessment once ready.Thanks !
Updated•12 years ago
|
Blocks: CVE-2012-4218
Updated•12 years ago
|
tracking-firefox17:
--- → ?
Updated•12 years ago
|
Assignee | ||
Comment 23•12 years ago
|
||
Comment on attachment 675005 [details] [diff] [review]
Fix
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 712600
User impact if declined: see comment 14 and dependent bug 767765
Testing completed (on m-c, etc.): Baked on trunk since 2012-10-25
Risk to taking this patch (and alternatives if risky): This is an area where there have been a number of fixes and regressions, so there is some risk, but I think it's acceptably small.
String or UUID changes made by this patch: None
Attachment #675005 -
Flags: approval-mozilla-beta?
Attachment #675005 -
Flags: approval-mozilla-aurora?
Comment 24•12 years ago
|
||
Comment on attachment 675005 [details] [diff] [review]
Fix
Please uplift today to branches so that this is in tomorrow's beta.
Attachment #675005 -
Flags: approval-mozilla-beta?
Attachment #675005 -
Flags: approval-mozilla-beta+
Attachment #675005 -
Flags: approval-mozilla-aurora?
Attachment #675005 -
Flags: approval-mozilla-aurora+
Updated•12 years ago
|
status-firefox17:
--- → affected
Assignee | ||
Comment 25•12 years ago
|
||
Comment 26•12 years ago
|
||
(In reply to Paul Silaghi [QA] from comment #20)
> Saw the issue on 19.0a1 (2012-10-24).
> Verified fixed on 19.0a1 (2012-10-28) on Win 7, Ubuntu 12.04 and Mac OS X
> 10.7.5
Verified fixed on FF 17b5, FF 18.0a2 (2012-11-13).
You need to log in
before you can comment on or make changes to this bug.
Description
•