900997 - Land "part 2" from bug 876155

Status: RESOLVED WORKSFORME

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

Description

[Mats Palmgren (inactive)]

This bug is about landing "part 2" from bug 876155 after bug 836925
is resolved.

The bug is hidden because the patch reveals further details about
how to trigger the crash in 876155.  It's sec-other because the
security problem was fixed by the patch that landed in that bug.

Comment 1

[Mats Palmgren (inactive)]

Sorry, I didn't mean to carry over all the flags from 876155.
Resetting all except "24/25:affected" (for the assertion mentioned in bug
876155 comment 14).  This bug is likely to be wontfix for branches though.

Comment 2

[Mats Palmgren (inactive)]

Replicating the "part 2" review comment (bug 876155 comment 8)
just as a reminder:

::: content/base/src/DirectionalityUtils.cpp
@@ +506,5 @@
> +   * mTextNode the changed text node passed into ResetNodeDirection
> +   * mMustReset when true the dirAutoSetBy property of the element must change;
> +   *            when false it may retain its previous value
> +   */
> +  struct TextNodeForReset {

Make this a MOZ_STACK_CLASS please.

Comment 3

[Simon Montagu :smontagu]

Attachment: Patch for checkin with review comment addressed

Carrying over r=ehsan from bug 876155 comment 2 and sec-approva=abillings from bug 876155 comment 20.

Comment 4

[Mats Palmgren (inactive)]

Ehsan, do you think the patch here is still relevant now that bug 1228882 is fixed?

Comment 5

[(no longer active)]

(In reply to Mats Palmgren (:mats) from comment #4)
> Ehsan, do you think the patch here is still relevant now that bug 1228882 is
> fixed?

It's not immediately obvious to me if the patches have the same effect.  It is also not obvious to me what crash this is trying to fix, so I can't really answer this question.

Comment 6

[Mats Palmgren (inactive)]

See bug 876155 comment 3.  I believe "part 2" was to address the assertion.
When I load the testcase (note that Jesse's DOMFuzzLite add-on is required),
I get an error:
"Error: Exposing privileged or cross-origin callable is prohibited"
which comes from this call: fuzzPriv.forceGC();
so I guess the test isn't working anymore.

Comment 7

[Mats Palmgren (inactive)]

Attachment: mochitest patch

I hooked up the forceGC() call in the test to SpecialPowers.forceGC().
I don't see any assertion in a Linux debug build.

Comment 8

[Mats Palmgren (inactive)]

If you don't see any reason to land the "part 2" patch then we should probably
resolve this bug as WFM.  What do you think?

Note also bug 836925, which is another follow-up from bug 876155.
The testcase in that bug still triggers a fatal assertion though:
"clearedEntries == 0 (Map should be empty already)", at dom/base/DirectionalityUtils.cpp:545
(Perhaps that was the assertion Simon was talking about in
bug 876155 comment 3, I don't know.)

I figured it might be a good idea to resolve these bugs while you and
Peter has this code paged in...

Comment 9

[(no longer active)]

(In reply to Mats Palmgren (:mats) from comment #8)
> If you don't see any reason to land the "part 2" patch then we should
> probably
> resolve this bug as WFM.  What do you think?

Given the assertion doesn't happen any more, that sounds fine.

> Note also bug 836925, which is another follow-up from bug 876155.
> The testcase in that bug still triggers a fatal assertion though:
> "clearedEntries == 0 (Map should be empty already)", at
> dom/base/DirectionalityUtils.cpp:545
> (Perhaps that was the assertion Simon was talking about in
> bug 876155 comment 3, I don't know.)

Oh, that assertion is probably bad.  Worth someone (ideally not me ;-) looking into it.

> I figured it might be a good idea to resolve these bugs while you and
> Peter has this code paged in...

FWIW every time I look at this code, I just read <https://dxr.mozilla.org/mozilla-central/source/dom/base/DirectionalityUtils.cpp#8> to remember again how this stuff works, I can't really say I know more than what the comments there state after all of these years.  In hindsight, I'm not super happy about the complicated design we came up (even though it's not obvious how to implement this efficiently without such a design.)  I'm also not happy about all of the raw pointer dance we do in this code -- we should probably replace it all with strong pointers, and declare things to the cycle collector when we create cycles.

The blame in all of the former and part of the latter goes to me.  :(