1468665 - Need to be able to read the old style attribute value when DeclarationBlock callback is called

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

Bug 1466963 added callbacks to get called when something is about to change, but
callbacks can't actually read attribute values, in generic sense (they could just use RawServoUnlockedDeclarationBlock)
Need to be able to read DOM attributes values without going through some stylo specific stuff, since AttributeWillChange should get called during the callback, and we can't really control easily all the nsIMutationObserver implementations, and the whole point of this all is to make style attribute handling more consistent with other attributes.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Isn't Servo_UnlockedDeclarationBlock_GetCssText added in bug 1466963 meant to do this?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Apparently it was, but that isn't the right tool. Random nsIMutationObserver implementations can't get access to RawServoUnlockedDeclarationBlock instance.
Also, setAttribute("style", "foobar"); and then getAttribute("style") is supposed to return "foobar". So DOM API users really need to deal with DOM attributes (since nsAttrValue stores the string data in that case), not raw style system structs.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And even if one would manually try to get just the string value from the old nsAttrValue first, and if not set, take string value from RawServoUnlockedDeclarationBlock and then do a nsAttrValue, that would be too slow, since nsAttrValue swaps aren't fast enough. And, in general we don't want to do the swap, or the old value reading, but because of nsIMutationObserver API being rather flexible, we don't know whether someone will actually need the old value.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix_stylo_csstext.diff

This is emilio's code.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

For stuff not going through CSSOM, you should already be able to get cssText from the declaration block already, no?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Not sure what comment 5 means, but during the callback reading the string value of style attribute fails badly without the patch.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

The following happens without the patch:

#13 0x00007f246fe3fe38 in std::panicking::begin_panic (msg=..., file_line_col=0x7f2474bf6e58) at /checkout/src/libstd/panicking.rs:538
#14 0x00007f246fe3fd4e in atomic_refcell::AtomicBorrowRef::do_panic (borrow=0x7f2456f1a078, new=9223372036854775809) at third_party/rust/atomic_refcell/src/lib.rs:161
#15 0x00007f246fdd00d5 in atomic_refcell::AtomicBorrowRef::new (borrow=0x7f2456f1a078) at /home/smaug/mozilla/hg/push-m-i/third_party/rust/atomic_refcell/src/lib.rs:130
#16 0x00007f246fd8a849 in <atomic_refcell::AtomicRefCell<T>>::borrow (self=0x7f2456f1a078) at /home/smaug/mozilla/hg/push-m-i/third_party/rust/atomic_refcell/src/lib.rs:88
#17 0x00007f246f903282 in style::shared_lock::SharedRwLock::read (self=0x7f245c493640) at servo/components/style/shared_lock.rs:84
#18 0x00007f246f2db684 in geckoservo::glue::read_locked_arc (raw=0x7f24556e0948, func=...) at servo/ports/geckolib/glue.rs:1616
#19 0x00007f246f27ec55 in Servo_DeclarationBlock_GetCssText (declarations=0x7f24556e0948, result=0x7fff47c32f30) at servo/ports/geckolib/glue.rs:3416

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I'll let emilio to say, whether the patch is ready for a review.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I think it should be ok to get it reviewed if we really want GetAttr to work from AttrWillChange. It's not really ideal, but I can't think of anything better that doesn't involve passing an extra argument around and special-casing for that instead in a really ugly way.

It should remove the unused Unloked_.. GetCssText function though.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Of course AttrWillChange must be able to read attribute values. That is the whole point of AttrWillChange.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8985428 [details] [diff] [review]
fix_stylo_csstext.diff

removal of that unused version of GetCSSText should be a separate patch.

(curious, what is not ideal with this?)

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

That's exactly the reason I wanted to see its usage before reviewing the patches in bug 1466963 comment 16. It is very usual that you think something can be done but eventually proven it's not.

smaug, could you post your patch for what you want to fix somewhere and we can try explore a proper solution with all the bits taken into consideration?

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8985428 [details] [diff] [review]
fix_stylo_csstext.diff

Review of attachment 8985428 [details] [diff] [review]:
-----------------------------------------------------------------

This looks dangerous, and I'm not happy to do this unless it is really the only feasible approach.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

This blocks bug 1428246.

Note, I couldn't have even imagined that reading attribute value during the callback wouldn't be possible when looking at bug 1466963, since such limitation is just weird.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

The key point is that any random nsIMutationObserver::AttributeWillChange implementation must be able to read style attribute value as string. That is the API contract.
And since cloning of DeclarationBlock is very slow, we need to be able to find a way for that without cloning.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #14)
> Note, I couldn't have even imagined that reading attribute value during the
> callback wouldn't be possible when looking at bug 1466963, since such
> limitation is just weird.

Sure, there are all kinds of invariation people couldn't come up with all the time. I failed to notice that restriction you may be hitting at that time either. However, you could test the patch with your proposed change before the patches sent to review there, and that was what I wanted to see before embedding those complexity into the codebase.

Let me think a bit more about this.

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #15)
> The key point is that any random nsIMutationObserver::AttributeWillChange
> implementation must be able to read style attribute value as string. That is
> the API contract.

But you cannot try locking something which is already locked for mutating. There wasn't supposed to be a callback at that point at all. It is adding the callback at the point the lock is being hold which breaks all the invariants.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

DOM doesn't care about some stylo internal locks. DOM needs to know when a declaration value is about to change and when it has changed. How the notifications are implemented on stylo side isn't relevant to DOM (except that performance needs to be good).

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Anyway, let's try designing the fix from the very beginning.

From the patch in bug 1466963, it seems set_property doesn't really need to be invoked when the lock is held if the unlocked declaration block is useless. The complexity is completely due to remove_property.

The remove_property function in glue.rs is the only callsite of DeclarationBlock::remove_property, this makes me think that we can probably make remove_property a two phase function exactly like set_property.

Checking for longhand removal is very fast: you only need to check the longhand id set, and we have been doing this currently.

Checking for most shorthands should be reasonable fast as well, you can iterate all the longhands and see whether there is any in the longhand set. In majority of cases, that check should hit at the first longhand if the author used the shorthand from the very beginning. For "all" shorthand it might be horrible, but that's probably not very common and is optimizable if really necessary.

Checking for custom properties would be unfortunate, we would need to traverse the declaration block. That makes me think maybe we can move the searching loop to the first phase, and have it return a intermediate result that can be used in the second phase.

Given this, I guess we can split DeclarationBlock::remove_property into two methods: get_items_to_remove, which does the lookup and probably returns a Vec<usize>, and then a commit_remove_property which accepts Vec<usize>. If the allocation is a problem, maybe we can use enum { Longhand(usize), Shorthand(Vec<usize>) } instead or probably SmallVec<[usize; 1]>.

With the remove function split, we can either invoke the callback without lock, or just split the FFI functions and do the callback-ish stuff in the C++ side.

emilio, what do you think about this?

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19)
> Anyway, let's try designing the fix from the very beginning.
> 
> From the patch in bug 1466963, it seems set_property doesn't really need to
> be invoked when the lock is held if the unlocked declaration block is
> useless. The complexity is completely due to remove_property.

Well, there's the gotcha of that only being valid as long as we keep bug 1415330 in the tree, kind of... Chances are we still want to fire mutation events and such though, I guess, so sure, that's probably fine.

> The remove_property function in glue.rs is the only callsite of
> DeclarationBlock::remove_property, this makes me think that we can probably
> make remove_property a two phase function exactly like set_property.
> 
> Checking for longhand removal is very fast: you only need to check the
> longhand id set, and we have been doing this currently.
> 
> Checking for most shorthands should be reasonable fast as well, you can
> iterate all the longhands and see whether there is any in the longhand set.
> In majority of cases, that check should hit at the first longhand if the
> author used the shorthand from the very beginning. For "all" shorthand it
> might be horrible, but that's probably not very common and is optimizable if
> really necessary.
> 
> Checking for custom properties would be unfortunate, we would need to
> traverse the declaration block. That makes me think maybe we can move the
> searching loop to the first phase, and have it return a intermediate result
> that can be used in the second phase.
> 
> Given this, I guess we can split DeclarationBlock::remove_property into two
> methods: get_items_to_remove, which does the lookup and probably returns a
> Vec<usize>, and then a commit_remove_property which accepts Vec<usize>. If
> the allocation is a problem, maybe we can use enum { Longhand(usize),
> Shorthand(Vec<usize>) } instead or probably SmallVec<[usize; 1]>.
> 
> With the remove function split, we can either invoke the callback without
> lock, or just split the FFI functions and do the callback-ish stuff in the
> C++ side.

Splitting remove_property is an option.

Rather than doing that in a "grab everything to remove, then actually remove", I'd just make it something like:

enum WillRemove {
    No,
    Yes { starting_at: usize },
}

Then just basically resuming the remove operation from `starting_at`. That doesn't require any allocation at all. In any case that means you convert the remove operation from taking a lock to two (one read, one write, calling the callback in the meantime). But that would need to happen anyway with your model.

WDYT about that Xidorn?

I can work on something like that, though I think landing the existing patch would be fine if the bug this blocks gets review sooner than that (doesn't seem too complex to implement though, so probably that won't happen)

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #20)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19)
> > Anyway, let's try designing the fix from the very beginning.
> > 
> > From the patch in bug 1466963, it seems set_property doesn't really need to
> > be invoked when the lock is held if the unlocked declaration block is
> > useless. The complexity is completely due to remove_property.
> 
> Well, there's the gotcha of that only being valid as long as we keep bug
> 1415330 in the tree, kind of... Chances are we still want to fire mutation
> events and such though, I guess, so sure, that's probably fine.
> 
> > The remove_property function in glue.rs is the only callsite of
> > DeclarationBlock::remove_property, this makes me think that we can probably
> > make remove_property a two phase function exactly like set_property.
> > 
> > Checking for longhand removal is very fast: you only need to check the
> > longhand id set, and we have been doing this currently.
> > 
> > Checking for most shorthands should be reasonable fast as well, you can
> > iterate all the longhands and see whether there is any in the longhand set.
> > In majority of cases, that check should hit at the first longhand if the
> > author used the shorthand from the very beginning. For "all" shorthand it
> > might be horrible, but that's probably not very common and is optimizable if
> > really necessary.
> > 
> > Checking for custom properties would be unfortunate, we would need to
> > traverse the declaration block. That makes me think maybe we can move the
> > searching loop to the first phase, and have it return a intermediate result
> > that can be used in the second phase.
> > 
> > Given this, I guess we can split DeclarationBlock::remove_property into two
> > methods: get_items_to_remove, which does the lookup and probably returns a
> > Vec<usize>, and then a commit_remove_property which accepts Vec<usize>. If
> > the allocation is a problem, maybe we can use enum { Longhand(usize),
> > Shorthand(Vec<usize>) } instead or probably SmallVec<[usize; 1]>.
> > 
> > With the remove function split, we can either invoke the callback without
> > lock, or just split the FFI functions and do the callback-ish stuff in the
> > C++ side.
> 
> Splitting remove_property is an option.
> 
> Rather than doing that in a "grab everything to remove, then actually
> remove", I'd just make it something like:
> 
> enum WillRemove {
>     No,
>     Yes { starting_at: usize },
> }
> 
> Then just basically resuming the remove operation from `starting_at`. That
> doesn't require any allocation at all. In any case that means you convert
> the remove operation from taking a lock to two (one read, one write, calling
> the callback in the meantime). But that would need to happen anyway with
> your model.
> 
> WDYT about that Xidorn?

I think this is fine as well. I didn't suggest this because I didn't want to duplicate the loop code in both method, but I guess it's probably not that bad.

> I can work on something like that, though I think landing the existing patch
> would be fine if the bug this blocks gets review sooner than that (doesn't
> seem too complex to implement though, so probably that won't happen)

Is it that urgent to land? I'd rather we don't land the current patch at all. It should be delayed at most one day or two, and I think there still needs some time for the patch in bug 1428246 to get reviewed. This should be an orthogonal change. It just blocks the landing, not the patch itself.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #21)
> I think this is fine as well. I didn't suggest this because I didn't want to
> duplicate the loop code in both method, but I guess it's probably not that
> bad.

Yeah, the code is not very complicated, and this will make it simpler than what it currently is I think.

Great, I'll try that.

> > I can work on something like that, though I think landing the existing patch
> > would be fine if the bug this blocks gets review sooner than that (doesn't
> > seem too complex to implement though, so probably that won't happen)
> 
> Is it that urgent to land? I'd rather we don't land the current patch at
> all. It should be delayed at most one day or two, and I think there still
> needs some time for the patch in bug 1428246 to get reviewed. This should be
> an orthogonal change. It just blocks the landing, not the patch itself.

Probably not as much, sounds fine.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19)
> Given this, I guess we can split DeclarationBlock::remove_property into two
> methods: get_items_to_remove, which does the lookup and probably returns a
> Vec<usize>, and then a commit_remove_property which accepts Vec<usize>. If
> the allocation is a problem,
Random note, allocations are slow. If allocating in a hot code path, they definitely show up badly in the profiles.

Comment 24

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Olli Pettay [:smaug] from comment #23)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #19)
> > Given this, I guess we can split DeclarationBlock::remove_property into two
> > methods: get_items_to_remove, which does the lookup and probably returns a
> > Vec<usize>, and then a commit_remove_property which accepts Vec<usize>. If
> > the allocation is a problem,
> Random note, allocations are slow. If allocating in a hot code path, they
> definitely show up badly in the profiles.

It doesn't feel that removing properties would likely be a hot code to run, and if we use SmallVec with 1 slot inlined, we only need to allocate for removing shorthand, which is probably even rare, so I don't think it would cause performance issue at all. Also by pre-allocate with a reasonable capacity, e.g. min(longhand count of the shorthand, declaration block length), we can allocate only once for each removing operation.

Comment 25

[Phabricator Automation]

Attachment: Bug 1468665: Don't call the before change closure with the locked declaration block. r=xidorn

Comment 27

[Phabricator Automation]

Comment on attachment 8985897 [details]
Bug 1468665: Don't call the before change closure with the locked declaration block. r=xidorn

Xidorn Quan [:xidorn] UTC+10 has approved the revision.

https://phabricator.services.mozilla.com/D1681

Comment 28

[Phabricator Automation]

Comment on attachment 8985897 [details]
Bug 1468665: Don't call the before change closure with the locked declaration block. r=xidorn

Xidorn Quan [:xidorn] UTC+10 has requested changes to the revision.

https://phabricator.services.mozilla.com/D1681

Comment 29

[Phabricator Automation]

Comment on attachment 8985897 [details]
Bug 1468665: Don't call the before change closure with the locked declaration block. r=xidorn

Xidorn Quan [:xidorn] UTC+10 has approved the revision.

https://phabricator.services.mozilla.com/D1681

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/95b1c86a8b55
Don't call the before change closure with the locked declaration block. r=xidorn

Comment 31

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/95b1c86a8b55