fix: handle race conditions accessing elements in DOM

[rubenporras]

Fixed the browser to not appear to be 'ready' to set the body before the text to be set has been processed.

Otherwise, because the content is set asynchronously in a browser after the call to browser.setText, it can be that the first call to document.readyState is done before the browser has processed the request to modify the page, and therefore document.readyState returns true even if the page has no content. To fix this, we check that the document has at least one element before asking for document.readyState.

[rubenporras]

For #1396

@FlorianKroiss , @sebthom , I had some troubles implementing my proposed fix for #1396 because we are deriving from a class that is not supposed to be subclassed. I might still find some time to investigate further, but I think this PR should fix the exceptions already. What do you think?

[sebthom]

I don't understand how this is better or fundamentally different from what I proposed as part of #1449.

[FlorianKroiss]

This feels like a symptomatic fix to me, because now we probably won't get the exceptions, because the JavaScript expressions will always evaluate to something, but in cases where we previously had the errors, we now fallback to a default width/height, which will probably not fit the entire content once its actually loaded.

Maybe I can find some time this week to also have a go at this bug

[rubenporras]

This feels like a symptomatic fix to me, because now we probably won't get the exceptions, because the JavaScript expressions will always evaluate to something, but in cases where we previously had the errors, we now fallback to a default width/height, which will probably not fit the entire content once its actually loaded.

Maybe I can find some time this week to also have a go at this bug

I also would prefer a different solution. I will submit today what I was working on as a draft so that you can take a look.

You are right that we will now not get the exceptions but I think you are not in the part "we now fallback to a default width/height, which will probably not fit the entire content once its actually loaded": I think that this happens with and without the exception, that is what safeEvaluate achieves by catching any exceptions and logging them, then returning null. Then the code already handles null by falling back to the default calculation.

So my thinking is: if the code already handles exceptions by falling back, and we know why some exceptions happen, we should rather handlem them without polluting the log (since the handling is already in place).

Regarding your comment @sebthom , I think this solution is different to yours because it will only suppress the exceptions caused by this null access. Any other exceptions that might appear are still logged so that users/we can report them. In that sense is more fine grained.

[rubenporras]

@FlorianKroiss , #1463 is the code I was testing as a better solution. I run out of time today and did not had time to clean it up, so I will just for now comment what was the problem I had and where the PR is not finished.

[rubenporras]

@FlorianKroiss , @sebthom: I had some time to think about the problem again. I have updated the PR message to reflect what the problem and the fix is. Does this make sense now?

I will squash the commits before merging.

Besides that, all builds are failing now because if problems with the baseline check, which I do not understand because I think we have updated so far the versions correctly. Do you understand that part?

Have a nice weekend.