Comment 16 for bug 197781

Revision history for this message
In , Nickolay Ponomarev (asqueella) wrote :

I remember very little about this code, so can't delve into technical discussion now. I wrote what was my impression from trying to patch up this code.

> Shouldn't it suffice to just queue the overlays
That's what I'm doing. The hard thing about this code is that
a) the call graph is quite complicated on its own (supporting many different scenarios - fastload vs no fastload, initial load vs dynamic overlay, etc)
b) certain callbacks happen asynchronously from the event loop (meaning that the document may be touched in the interim)
c) it used to be the case that certain spots in the code could cause scripts to run (which could cause our code to be reentered).

You have to be sure you get to the end of the load process in any case - to call all overlay observers. And you have to be sure you handle the async notifications properly whatever order they come in (and be some notification always happens, so that you don't live in the loading state forever).

With the WIP#2 patch (I think) I had the problem with cancelled (BINDING_ABORTED) loads not being processed correctly; in normal cases it seemed to work correctly, but touching this code causes paranoia, as you see.

re scarecrow: possibly, but overlays are available to content, and the reentrancy issues and the fact that the script is able to make unexpected changes to document's state while it is loading make me feel uncomfortable. I'm mainly afraid of crashes here.

Maybe you're right and we should implement the simple fix and deal with corner cases in separate bugs. Feel free to take it :) I'll try to see if I still have my tests somewhere (not until Monday probably).

A patch like this feels like an alpha material though.