Bug 1786571 Comment 23 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

(In reply to Jens Stutte [:jstutte] from comment #21)
> (In reply to Jens Stutte [:jstutte] from comment #20)
> > FWIW I added two paper-over checks for the cancel case, try run: https://treeherder.mozilla.org/jobs?repo=try&revision=14d68daa0594eaccff0ba39c6d9c145a7a61e8fc
> 
> Looks not bad, but I wait for your analysis before making further steps.

Can you update the patch in phabricator to make it easier to see the changes?  I think I've manually/mentally diffed the changes to just the guards on the cache creator which makes sense [for the first linked backout crash](https://treeherder.mozilla.org/logviewer?job_id=390670496&repo=autoland&lineNumber=2772), but I'm not sure I understand what changes would address [the second linked backout crash](https://treeherder.mozilla.org/logviewer?job_id=390670496&repo=autoland&lineNumber=2772) where the [CachePromiseHandler::ResolvedCallback calls into MaybeExecuteFinishedScripts](https://searchfox.org/mozilla-central/rev/d8d75a05deaba7e89a6f1d4c1f197d01e2361d43/dom/workers/loader/CacheLoadHandler.cpp#64).  I presume if we do something to avoid [getting to CachePromiseHandler's creation in NetworkLoadHandler::PrepareForRequest](https://searchfox.org/mozilla-central/rev/d8d75a05deaba7e89a6f1d4c1f197d01e2361d43/dom/workers/loader/NetworkLoadHandler.cpp#365) then that will fix it, but I'm not sure what changed to ensure that.  If you could clarify, that would be great, thank you!  I've triggered a bunch more of the dt2 tests on try to give some extra confidence about the replication on that.
(In reply to Jens Stutte [:jstutte] from comment #21)
> (In reply to Jens Stutte [:jstutte] from comment #20)
> > FWIW I added two paper-over checks for the cancel case, try run: https://treeherder.mozilla.org/jobs?repo=try&revision=14d68daa0594eaccff0ba39c6d9c145a7a61e8fc
> 
> Looks not bad, but I wait for your analysis before making further steps.

Can you update the patch in phabricator to make it easier to see the changes?  I think I've manually/mentally diffed the changes to just the guards on the cache creator which makes sense [for the first linked backout crash](https://treeherder.mozilla.org/logviewer?job_id=390675417&repo=autoland&lineNumber=3623), but I'm not sure I understand what changes would address [the second linked backout crash](https://treeherder.mozilla.org/logviewer?job_id=390670496&repo=autoland&lineNumber=2772) where the [CachePromiseHandler::ResolvedCallback calls into MaybeExecuteFinishedScripts](https://searchfox.org/mozilla-central/rev/d8d75a05deaba7e89a6f1d4c1f197d01e2361d43/dom/workers/loader/CacheLoadHandler.cpp#64).  I presume if we do something to avoid [getting to CachePromiseHandler's creation in NetworkLoadHandler::PrepareForRequest](https://searchfox.org/mozilla-central/rev/d8d75a05deaba7e89a6f1d4c1f197d01e2361d43/dom/workers/loader/NetworkLoadHandler.cpp#365) then that will fix it, but I'm not sure what changed to ensure that.  If you could clarify, that would be great, thank you!  I've triggered a bunch more of the dt2 tests on try to give some extra confidence about the replication on that.

Back to Bug 1786571 Comment 23