bidi reftest 779003, 712600 broken with dynamic changes

VERIFIED FIXED in Firefox 17

Status

()

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: vlad, Assigned: smontagu)

Tracking

Trunk
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17+ verified, firefox18+ verified)

Details

Attachments

(6 attachments)

Created attachment 663468 [details]
dynamic version of 779003

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.
Created attachment 665529 [details] [diff] [review]
mark this reftest as random

David, Simon, are either/both of you ok with marking this reftest as random for now?
Attachment #665529 - Flags: review?(smontagu)
Created attachment 665662 [details]
dynamic version of 712600-1

Hrm.  Other tests fail in this same way -- dynamic version of 712600-1.
Created attachment 665663 [details]
dynamic version of 712600-2
Blocks: 779003, 712600
status-firefox18: --- → affected
tracking-firefox18: --- → ?
Summary: reftest 779003-1.html broken with dynamic changes → bidi reftest 779003, 712600 broken with dynamic changes

Comment 5

6 years ago
Did you mean to set this as tracking?
(Assignee)

Comment 6

6 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+
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]
Assignee: vladimir → smontagu
Whoops, I didn't actually press Save Changes when I did all that to the bug.. sorry!
Assignee: smontagu → bugs
Assignee: bugs → smontagu
Clearing the tracking-firefox18 flag, as I think it was set by mistake ?!?! Please renominate if it needs to be tracked .
status-firefox18: affected → ---
tracking-firefox18: ? → -
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
tracking-firefox18: - → ?
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 ?
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.
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

6 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

6 years ago
tracking-firefox18: ? → +
(Assignee)

Comment 15

6 years ago
Created attachment 675005 [details] [diff] [review]
Fix

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

6 years ago
Created attachment 675006 [details] [diff] [review]
Tests

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 18

6 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]
https://hg.mozilla.org/mozilla-central/rev/b339983c1dd5
https://hg.mozilla.org/mozilla-central/rev/8a9563705dac
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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
(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

6 years ago
Blocks: 767765

Updated

6 years ago
tracking-firefox17: --- → ?

Updated

6 years ago
tracking-firefox17: ? → +
(Assignee)

Comment 23

6 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 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+
status-firefox17: --- → affected
(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).
status-firefox17: fixed → verified
status-firefox18: fixed → verified
(Assignee)

Updated

6 years ago
Blocks: 775687
You need to log in before you can comment on or make changes to this bug.