1523581 - Remove usage of thread observers when firing the "load" event

Status: RESOLVED WONTFIX

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

Description

[Yaron Tausky [:ytausky]]

Using that mechanism is brittle when nested event loops come into play.

Comment 1

[Yaron Tausky [:ytausky]]

Attachment: Bug 1523581, part 1: Use RefPtr to manage lifetime of SheetLoadData

In order to avoid memory problems later on in the refactoring, this commit
replaces the explicit usage of NS_ADDREF and NS_RELEASE with RefPtr.
VerifySheetReadyToParse was moved from SheetLoadData to StreamLoader (its only
user) to avoid an object releasing itself.

Comment 2

[Boris Zbarsky [:bzbarsky]]

What is the plan for addressing the actual web use cases this mechanism is addressing? What do other browsers do in terms of possibly observing intermediate state that we are trying to not have observable here?

As far as the part 1 patch, I am somewhat unlikely to get to it until at least end of next week. It needs some really careful review.

Comment 3

[Yaron Tausky [:ytausky]]

The general plan is to collect all the rendering-affecting operations from the current task, add all the load events on top of that, and execute them all together, either immediately or in a separate task (according to the standard). It's kinda vague still, because I'm still trying to make sure I understand what all the possible paths are.

The patch I uploaded is pretty much independent from that; I did it first to make sure things don't break later on, but I think it's also valuable on its own.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't see how this is indepenent; without the rest of the plan, this patch will introduce flashes of "incorrect" content, no?

Comment 5

[Yaron Tausky [:ytausky]]

No, the current patch just replaces manual refcounting (NS_ADDREF and NS_RELEASE) with RefPtr. It doesn't change the order of operations. (At least not intentionally -- I was having some trouble matching some of the NS_ADDREFs to a corresponding NS_RELEASE, so it's possible I missed some subtler point there.)

Comment 6

[Yaron Tausky [:ytausky]]

After working on this for a while I realized that it's not going to be as easy as I hoped. Due to the various constraints at play here, the only semantics that make sense for synchronous style changes are that the task that fires the load event is scheduled immediately after the task that made the style changes, i.e. almost the same semantics as those of the current implementation (barring things like Long Task API).

Since the main point of this exercise was to unblock a different task, I will try a different approach and abandon this one for now. I still think the part 1 patch is generally useful, but I won't be able to come back to it in the coming weeks.