1120906 - Disable pre-loading of about:newtab for opened tabs

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Teodor Druta]

Attachment: windowsjs_dump.txt

This issue can be reproduced by opening about:newtab in a new tab and calling waitForPageLoad() right after. The new Window object is not added to the _windows map resulting in hasPageLoaded() windows.js method being called endlessly in a loop.

Comment 1

[Teodor Druta]

Attachment: 0001-Bug-1120906-Mozmill-fails-to-add-a-new-Window-object.patch

This is my proposal for fixing this issue, I think that we should call the updatePageLoadStatus() for the window Id inside hasPageLoaded method in the case of this.contains(aId) being false. This will update the _windows map with the new Window object.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8548152 [details] [diff] [review]
0001-Bug-1120906-Mozmill-fails-to-add-a-new-Window-object.patch

Review of attachment 8548152 [details] [diff] [review]:
-----------------------------------------------------------------

What will happen

::: mozmill/mozmill/extension/resource/modules/windows.js
@@ +130,5 @@
> +    else {
> +      if (!this.contains(aId)) {
> +        map.updatePageLoadStatus(aId, true);
> +      }
> +    }

Here you're forcing it to pass, what happens if the window doesn't loaded for real?
We never get notified in observer or we really fail in appending the window to the list?
Please add some dumps here to check if we really don't get notified when the window get's opened:
https://github.com/mozilla/mozmill/blob/2.0rc6/mozmill/mozmill/extension/resource/modules/windows.js#L139

Comment 3

[Teodor Druta]

(In reply to Cosmin Malutan from comment #2)
> We never get notified in observer or we really fail in appending the window
> to the list?
> Please add some dumps here to check if we really don't get notified when the
> window get's opened:
> https://github.com/mozilla/mozmill/blob/2.0rc6/mozmill/mozmill/extension/
> resource/modules/windows.js#L139

It seems that we don't get notified by the observer => the events are never attached on the window, which imo does not really exists in the ._windows map.

Comment 4

[Teodor Druta]

I was wrong when thinking that this is an issue with Mozmill failing to add a new Window object on the _windows map when opening a new tab. This has to do "browser.newtab.preload" property, somehow this property makes waitForPageLoad to fail intermittently with status=complete. So setting this property to false seems to help, I'll just stick to that developing the about:newtab incontent page class for now.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Teodor, maybe we should disable that preference by default with Mozmill. Can you have a look if that could be feasible? I don't want to regress something, but the impact sounds kinda low here.

Comment 6

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> Teodor, maybe we should disable that preference by default with Mozmill. Can
> you have a look if that could be feasible? I don't want to regress
> something, but the impact sounds kinda low here.

I tried to disable this preference in a local Mozmill instance, ran a few testruns, it seems to run ok, not affecting (negatively) any other test we have. I also think that disabling this preference by default should resolve a lot of other similar waitForPageLoad failures we have in our tests. I'll come with a patch proposal next, then.

Comment 7

[Teodor Druta]

Attachment: 0001-Bug-1120906-Initialize-browser.newtab.preload-proper.patch

This is the patch for disabling the browser.newtab.preload by default in mozmill.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8550250 [details] [diff] [review]
0001-Bug-1120906-Initialize-browser.newtab.preload-proper.patch

Review of attachment 8550250 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine. Please make sure to sort alphabetically before asking for review.

Comment 9

[Teodor Druta]

Attachment: 0001-Bug-1120906-Initialize-browser.newtab.preload.patch

Sorted preferences alphabetically.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8553058 [details] [diff] [review]
0001-Bug-1120906-Initialize-browser.newtab.preload.patch

Review of attachment 8553058 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me. Thanks for the fix Teodor!

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed on master as:
https://github.com/mozilla/mozmill/commit/3563bc0aee1b3d2cde9ed8707a7e650cab05eab1

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Important pref change, which we can push with the upcoming 2.0.10 release of mozmill.

Cherry-picked for hotfix-2.0 branch:
https://github.com/mozilla/mozmill/commit/770a26cb71d218bc003670bc175c5c97a0b8e668