793233 - bidi reftest 779003, 712600 broken with dynamic changes

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: 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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: mark this reftest as random

David, Simon, are either/both of you ok with marking this reftest as random for now?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: dynamic version of 712600-1

Hrm.  Other tests fail in this same way -- dynamic version of 712600-1.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: dynamic version of 712600-2

Comment 5

[(no longer active)]

Did you mean to set this as tracking?

Comment 6

[Simon Montagu :smontagu]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Whoops, I didn't actually press Save Changes when I did all that to the bug.. sorry!

Comment 9

[bhavana bajaj [:bajaj]]

Clearing the tracking-firefox18 flag, as I think it was set by mistake ?!?! Please renominate if it needs to be tracked .

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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...

Comment 11

[bhavana bajaj [:bajaj]]

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 ?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

[bhavana bajaj [:bajaj]]

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 ?

Comment 14

[Simon Montagu :smontagu]

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.

Comment 15

[Simon Montagu :smontagu]

Attachment: 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.

Comment 16

[Simon Montagu :smontagu]

Attachment: 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.

Comment 17

[Simon Montagu :smontagu]

https://tbpl.mozilla.org/?tree=Try&rev=cb6ac15fc926

Comment 18

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b339983c1dd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9563705dac

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b339983c1dd5
https://hg.mozilla.org/mozilla-central/rev/8a9563705dac

Comment 20

[Paul Silaghi, QA [:pauly]]

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

Comment 21

[bhavana bajaj [:bajaj]]

(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 !

Comment 23

[Simon Montagu :smontagu]

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

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 675005 [details] [diff] [review]
Fix

Please uplift today to branches so that this is in tomorrow's beta.

Comment 25

[Simon Montagu :smontagu]

https://hg.mozilla.org/releases/mozilla-aurora/rev/78169f1caa36
https://hg.mozilla.org/releases/mozilla-aurora/rev/c8f3d5623567

https://hg.mozilla.org/releases/mozilla-beta/rev/91eb4a93654c
https://hg.mozilla.org/releases/mozilla-beta/rev/21195255f82a

Comment 26

[Paul Silaghi, QA [:pauly]]

(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).