Closed Bug 1448535 Opened 6 years ago Closed 5 years ago

Remove PresShell arena-related code from style structs and ComputedStyle

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1525371
Tracking Flags:
Tracking Status
firefox61 --- affected

People

(Reporter: xidorn, Unassigned)

References

Details

Attachments

(5 files)

Bug 1448535 part 1 - Make nsMathMLmtdInnerFrame use UniquePtr for its StyleText.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1448535 part 2 - Remove arena-related code from style structs.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1448535 part 3 - Remove arena-related code from ComputedStyle.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1448535 part 4 - Remove ArenaRefPtr.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1448535 part 5 - Remove ArenaObjectID of objects only used in the old style system.
59 bytes, text/x-review-board-request
emilio
: review+
Details 
Since we don't use the arena anymore.
Assignee: nobody → xidorn+moz
Blocks: 1448728
Comment on attachment 8962224 [details]
Bug 1448535 part 3 - Remove arena-related code from ComputedStyle.

https://reviewboard.mozilla.org/r/231058/#review236512

So the reason why `ArenaRefPtr<ComputedStyle>` needs to exist is that the ComputedStyle keeps a reference to nsPresContext, and can't outlive it, that's why the cached style context in `nsComputedDOMStyle` can't go away, unless we have another mechanism to prevent that now?
Attachment #8962224 - Flags: review?(emilio)
Comment on attachment 8962223 [details]
Bug 1448535 part 2 - Remove arena-related code from style structs.

https://reviewboard.mozilla.org/r/231056/#review236514
Attachment #8962223 - Flags: review?(emilio) → review+
Comment on attachment 8962226 [details]
Bug 1448535 part 5 - Remove ArenaObjectID of objects only used in the old style system.

https://reviewboard.mozilla.org/r/231062/#review236516

::: layout/base/nsPresArena.cpp
(Diff revision 1)
>          aSizes.mArenaSizes.mLineBoxes += totalSize;
>          break;
> -      case eArenaObjectID_nsRuleNode:
> -        aSizes.mArenaSizes.mRuleNodes += totalSize;
> -        break;
> -      case eArenaObjectID_GeckoComputedStyle:

I think this one needs to stay per my review above.
Attachment #8962226 - Flags: review?(emilio)
Attachment #8962225 - Flags: review?(bzbarsky)
Comment on attachment 8962226 [details]
Bug 1448535 part 5 - Remove ArenaObjectID of objects only used in the old style system.

https://reviewboard.mozilla.org/r/231062/#review236516

> I think this one needs to stay per my review above.

I don't think the one needs to stay is this line. This should be able to go away. The only thing may need to stay is the line in nsPresArenaObjectList.h. I can nuke part 3 and 4, and only do the other three for now.
Comment on attachment 8962224 [details]
Bug 1448535 part 3 - Remove arena-related code from ComputedStyle.

Per discussion with emilio on IRC, setting review flag again.

So, I don't think the comment in comment 6 is a problem, because every access to mComputedStyle in nsComputedDOMStyle is protected behind UpdateCurrentStyleSources, which is supposed to clear the pointer if the document or the presshell has gone.
Attachment #8962224 - Flags: review?(emilio)
Attachment #8962225 - Flags: review?(bzbarsky)
Attachment #8962226 - Flags: review?(emilio)
Comment on attachment 8962222 [details]
Bug 1448535 part 1 - Make nsMathMLmtdInnerFrame use UniquePtr for its StyleText.

https://reviewboard.mozilla.org/r/231054/#review236640

r=me, optional nit:

::: layout/mathml/nsMathMLmtableFrame.cpp:1273
(Diff revision 1)
>  
>  nsMathMLmtdInnerFrame::nsMathMLmtdInnerFrame(ComputedStyle* aStyle)
>    : nsBlockFrame(aStyle, kClassID)
> -{
>    // Make a copy of the parent nsStyleText for later modification.
> -  mUniqueStyleText = new (PresContext()) nsStyleText(*StyleText());
> +  , mUniqueStyleText(new nsStyleText(*StyleText()))

I think you can use MakeUnique<nsStyleText>(*StyleText()) here, rather than "new" -- if so, that's probably better for consistency & general-moving-away-from-new/delete.
Attachment #8962222 - Flags: review?(dholbert) → review+
Comment on attachment 8962224 [details]
Bug 1448535 part 3 - Remove arena-related code from ComputedStyle.

https://reviewboard.mozilla.org/r/231058/#review236820

::: layout/style/nsComputedDOMStyle.h:756
(Diff revision 1)
>     * in ClearCurrentStyleSources.  If we resolved one ourselves, then
>     * ClearCurrentStyleSources leaves it in mComputedStyle for use the next
>     * time this nsComputedDOMStyle object is queried.  UpdateCurrentStyleSources
> -   * in this case will check that the style context is still valid to be used,
> +   * in this case will check that the computed style is still valid to be used,
>     * by checking whether flush styles results in any restyles having been
>     * processed.

Please extend this comment to mention that this computed style can keep a dangling reference before UpdateCurrentStyleSources is called, and that it's completely unsafe to poke at it without first having called that.

Also, please:

  MOZ_RELEASE_ASSERT(mComputedStyle->PresContext() == mPresShell->GetPresContext());
  
When retaining the cached computed style.
Attachment #8962224 - Flags: review?(emilio) → review+
Comment on attachment 8962226 [details]
Bug 1448535 part 5 - Remove ArenaObjectID of objects only used in the old style system.

https://reviewboard.mozilla.org/r/231062/#review236822
Attachment #8962226 - Flags: review?(emilio) → review+
Comment on attachment 8962225 [details]
Bug 1448535 part 4 - Remove ArenaRefPtr.

https://reviewboard.mozilla.org/r/231060/#review236910
Attachment #8962225 - Flags: review?(bzbarsky) → review+
Comment on attachment 8962224 [details]
Bug 1448535 part 3 - Remove arena-related code from ComputedStyle.

https://reviewboard.mozilla.org/r/231058/#review236820

> Please extend this comment to mention that this computed style can keep a dangling reference before UpdateCurrentStyleSources is called, and that it's completely unsafe to poke at it without first having called that.
> 
> Also, please:
> 
>   MOZ_RELEASE_ASSERT(mComputedStyle->PresContext() == mPresShell->GetPresContext());
>   
> When retaining the cached computed style.

This is not going to work, because `mComputedStyle` can very much use a different PresShell than `mPresShell` if the content is in a different document than where `getComputedStyle` is invoked.

That said, it's probably risky, since the other PresShell may have died without us noticing. Maybe we should have `mPresShell` points to the PresShell which generates the computed style.
Comment on attachment 8962224 [details]
Bug 1448535 part 3 - Remove arena-related code from ComputedStyle.

https://reviewboard.mozilla.org/r/231058/#review236820

> This is not going to work, because `mComputedStyle` can very much use a different PresShell than `mPresShell` if the content is in a different document than where `getComputedStyle` is invoked.
> 
> That said, it's probably risky, since the other PresShell may have died without us noticing. Maybe we should have `mPresShell` points to the PresShell which generates the computed style.

Hmm, yeah. We could assert either mPresShell or presShellForContent's shell for now I guess...

I guess this means that we're looking at the wrong undisplayed style generation, and that that can make us retain the style incorrectly, and thus this patch is not really correct, right?
Comment on attachment 8962224 [details]
Bug 1448535 part 3 - Remove arena-related code from ComputedStyle.

https://reviewboard.mozilla.org/r/231058/#review236820

> Hmm, yeah. We could assert either mPresShell or presShellForContent's shell for now I guess...
> 
> I guess this means that we're looking at the wrong undisplayed style generation, and that that can make us retain the style incorrectly, and thus this patch is not really correct, right?

Right, that means we need to fix other stuff to make it correct.
I'd like to have part 1 and part 2 land first since other bugs may depend on them, and they are not controversial.
Keywords: leave-open
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/341003f5c05f
part 1 - Make nsMathMLmtdInnerFrame use UniquePtr for its StyleText. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/4a8297ea8c0a
part 2 - Remove arena-related code from style structs. r=emilio
Priority: -- → P3
Unassigning myself since I'm not actively working on this.
Assignee: xidorn+moz → nobody

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?

Flags: needinfo?(svoisen)
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(svoisen)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: