1265769 - Don't accidentally enable meta refresh timers in nsDocShell::SetIsActive() if the page has not loaded

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

+++ This bug was initially created as a clone of Bug #518805 +++

This bug is for addressing the incorrect enabling of timers according to https://bugzilla.mozilla.org/show_bug.cgi?id=518805#c27

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't enable meta refresh timers if the page has not finished loading

This is a regression from bug 518805

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b02352b6d9bb

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8742889 [details] [diff] [review]
Don't enable meta refresh timers if the page has not finished loading

This isn't quite right either.  SuspendRefreshURIs when we go inactive will suspend all our descendant docshells.  Skipping ResumeRefreshURIs will not resume them, therefore.

Now we _have_ already done SetIsActive stuff on our descendants before we reach this code... but we don't do that for mozbrowsers.

So even with this patch we end up with two remaining issues:

1)  A mozbrowser descendant of this docshell that's done loading will not have its refresh stuff correctly reenabled.  Or more precisely, it should never have been disabled to start with.

2)  If we _are_ done loading we're now walking all kids twice in SetIsActiveInternal: once to SetIsActive* on them, and once under ResumeRefreshURIs/SuspendRefreshURIs.  This means we end up with behavior exponential in the depth of the docshell tree.  Luckily this is capped at 10 or so, so we don't get to experience the true power of 2^{50} or whatnot, but that's still not great...

Seems to me that given the active/inactive semantics around mozbrowser, calling we want to call versions of Resume/SuspendRefreshURIs here that do NOT do anything with the kids, and let the walk over kids earlier in this method handle the kids as needed.  In the case of ResumeRefreshURIs that means just calling RefreshURIFromQueue.  In the case of SuspendRefreshURIs it means either adding an argument to it or factoring out the first part of it into a separate method that we can call here.  Probably the latter, since SuspendRefreshURIs is public API.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.