Resource fetch timing information is stored too late during document navigation

[jdm]

Given the changes in #24466 (or substituting timing() without them), it is easy to get a page to panic by checking navigation.timing before parsing is complete. More specifically, the panic occurs because in this code there is no navigation timing entry that has been reported when a page like this is loaded:

<script>
console.log(performance.timing)
</script>

This occurs because we only register performance entries when a response is complete, rather than when the first bytes are received. I think we need to update all of the resource timing code to submit twice - when the first bytes are received, and then update the existing entry when all of the final timings are available.

We could:

• add a ResourceFetchTiming argument to the Result passed in process_response
• call submit_resource_timing after creating the document parser
• make Performance::queue_entries return the index of the added performance entry in self.entries
• add a update_entry method to Performance that accepts an index and a &PerformanceEntry value and overwrites the corresponding entry in self.entries
• call the new update_entry API from process_response_eof

[tomasdivito]

I'll try to work on this as soon as I can :)
@highfive: assign me

[highfive]

Hey @tomasdivito! Thanks for your interest in working on this issue. It's now assigned to you!

[tomasdivito]

@jdm I have a couple questions regarding to your suggested solution.

add a ResourceFetchTiming argument to the Result passed in process_response
Not pretty sure what this mean, and what would be the point on doing so...
fn process_response(&mut self, meta_result: Result<FetchMetadata, NetworkError>) you mean something like instead making something like Result<(FetchMetadata, ResourceFetchTiming), NetworkError> which I'm not that sure if it's actually valid and as I said, I don't know how would relate to the other steps...

make Performance::queue_entries return the index of the added performance entry in self.entries
queue_entry() receives the entry itself and the add_to_performance_entries_buffer
Should we return the index of the entry in the buffer when we push this entry:

        if add_to_performance_entries_buffer {
            self.buffer
                .borrow_mut()
                .entries
                .push(DomRoot::from_ref(entry));
        }

because we are also adding it to the observers... maybe it doesn't matter, but if that's the case and we get the index back (adding the return value to be -> usize who would have to carry this index and then pass it somehow to process_response_eof 🤔

Sorry for the amount of questions but I'm not pretty sure how this would work. I get what this would solve but I'm having some doubts and maybe this is a little bit more complicated.

[jdm]

Good questions! Looking more closely at the code, there is already a timing field inside Metadata, and that's part of the FetchMetadata enum, so we should be able to rely on that in the process_response implementations.

As for the question about queue_entry, since every caller of queue_entry passes true for add_to_performance_entries_buffer, let's remove that argument and always push the entry. Then we will always have an index that we can return at the end of the method.

[tomasdivito]

Just as an update, I was not able to focus on this because I was too busy with university.

I will work on this saturday and will create a Pull Request for this.

Again sorry! I would love to work on this more but at least until december I have no much free time :(

[jdm]

Thanks for letting us know!

[tomasdivito]

@jdm As I had a harder time than I thought with exams, which won't end until the second week of december, I may had to leave this patch be! And maybe the issue and PR from #24466

I'll surely will be back to do these if nobody made it, or find something else! Sorry :(

[shnmorimoto]

@highfive: assign me

[highfive]

Hey @shnmorimoto! Thanks for your interest in working on this issue. It's now assigned to you!

[CYBAI]

Fixed by #25205