Closed Bug 1164728 Opened 9 years ago Closed 9 years ago

Intermittent browser_net_autoscroll.js | Test timed out

Categories

(DevTools :: Netmonitor, defect)

Product:
DevTools
Component:
Netmonitor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 unaffected, firefox40 disabled, firefox41 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

Status:
RESOLVED FIXED
Milestone:
Firefox 41
Tracking Flags:
Tracking Status
firefox39 --- unaffected
firefox40 --- disabled
firefox41 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: KWierso, Assigned: bgrins)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

autoscroll-sync.patch
7.70 KB, patch
vporof
: review+
Details | Diff | Splinter Review 
16:31:02 INFO - "UPDATING_RESPONSE_CONTENT": 1,
16:31:02 INFO - "RECEIVED_RESPONSE_CONTENT": 0,
16:31:02 INFO - "UPDATING_EVENT_TIMINGS": 1,
16:31:02 INFO - "RECEIVED_EVENT_TIMINGS": 0
16:31:02 INFO - }
16:31:02 INFO - }
16:31:02 INFO - 4547 INFO Longer timeout required, waiting longer... Remaining timeouts: 1
16:31:02 INFO - 4548 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_autoscroll.js | Test timed out - expected PASS
16:31:02 INFO - 4549 INFO finish() was called, cleaning up...
16:31:02 INFO - MEMORY STAT vsize after test: 984395776
16:31:02 INFO - MEMORY STAT residentFast after test: 280195072
16:31:02 INFO - MEMORY STAT heapAllocated after test: 103856984
16:31:02 INFO - 4550 INFO TEST-OK | browser/devtools/netmonitor/test/browser_net_autoscroll.js | took 90753ms
16:31:02 INFO - 4551 INFO TEST-UNEXPECTED-FAIL | browser/devtools/netmonitor/test/browser_net_autoscroll.js | Found a tab after previous test timed out: http://example.com/browser/browser/devtools/netmonitor/test/html_infinite-get-page.html - expected PASS
16:31:02 INFO - --DOCSHELL 0x7f80fc853800 == 13 [pid = 3630] [id = 30]
16:31:02 INFO - [3630] WARNING: NS_ENSURE_TRUE(mMutable) failed: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/netwerk/base/nsSimpleURI.cpp, line 264
16:31:02 INFO - [3630] WARNING: Page was shift reloaded, skipping ServiceWorker control: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/dom/base/nsDocument.cpp, line 4809
16:31:02 INFO - [3630] WARNING: NS_ENSURE_TRUE(!mHasOrHasHadOwnerWindow || mOwnerWindow) failed: file ../../dist/include/mozilla/DOMEventTargetHelper.h, line 131
16:31:02 INFO - [3630] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file /builds/slave/m-in-l64-d-0000000000000000000/build/src/docshell/base/nsDocShell.cpp, line 4591
16:31:02 INFO - --DOCSHELL 0x7f80f49de000 == 12 [pid = 3630] [id = 29]
16:31:02 INFO - --DOCSHELL 0x7f80f384e800 == 11 [pid = 3630] [id = 28]
16:31:02 INFO - ++DOCSHELL 0x7f80f1733800 == 12 [pid = 3630] [id = 31]
16:31:02 INFO - ++DOMWINDOW == 34 (0x7f80f1a61c00) [pid = 3630] [serial = 78] [outer = (nil)]
16:31:02 INFO - ++DOMWINDOW == 35 (0x7f80f4129400) [pid = 3630] [serial = 79] [outer = 0x7f80f1a61c00]
Summary: Intermittent browser_net_autoscroll.js | Found a tab after previous test timed out: html_infinite-get-page.html → Intermittent browser_net_autoscroll.js | Test timed out
Depends on: 1143224
This should almost certainly fix any intermittents, and it also improves the UX by getting rid of any lag between items being inserted and the scroll snapping to the bottom.  It takes advantage of the fact that requests are queued up on the frontend and processed in a loop in second part in Bug 1143224.

Since the requests are batch inserted, we can have a more solid feeling scroll functionality by doing the scroll after the batch of requests finishes.  It makes it feel more snappy and it should never 'miss' a user initiated scroll.

I'm removing the autoscrollWithAppendedItems option from SideMenuWidget, but only because it makes things much simpler (since we know that multiple inserts are happening in sequence, but as far as the SMW knows each insert is discreet).  The netmonitor was the only place this feature was used.

The other option would be to spin the event loop and scroll right away after that, but it reintroduces the lag and has the risk of missing user scrolls if they happen in between the insertions and the scroll being set.

I thought also about adding in startBatchInsert() and stopBatchInsert() functions in the widget that would wrap this functionality up but it felt sort of weird.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=128f438c1a9f
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8621736 - Flags: review?(vporof)
Attachment #8621736 - Flags: review?(vporof) → review+
Keywords: checkin-needed
remote:   https://hg.mozilla.org/integration/fx-team/rev/0a6778a3d65a
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0a6778a3d65a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Please nominate this for Aurora approval when you get a chance.
status-firefox39: --- → unaffected
status-firefox40: --- → affected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
Flags: needinfo?(bgrinstead)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #147)
> Please nominate this for Aurora approval when you get a chance.

I don't know that we will want to uplift the second patch from Bug 1143224, which this depends on.  It's more involved than this one, and could at least use some time to bake.
Flags: needinfo?(bgrinstead)
This is happening too frequently to ignore. If we can't reasonably uplift a fix for the failure, we can skip the test otherwise. Let me know what you want to do.
Flags: needinfo?(bgrinstead)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #150)
> This is happening too frequently to ignore. If we can't reasonably uplift a
> fix for the failure, we can skip the test otherwise. Let me know what you
> want to do.

I'd rather let 1143224 ride the train, so I'd opt for skipping the test on 40.  The feature is pretty isolated within the SideMenuWidget and unlikely to break due to an uplift.
Flags: needinfo?(bgrinstead)
Flags: needinfo?(ryanvm)
status-firefox40: affected → disabled
Flags: needinfo?(ryanvm)
Comments 155-161 were erroneous stars. They were legit bustage from bug 1175678.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #162)
> Comments 155-161 were erroneous stars. They were legit bustage from bug
> 1175678.

Whew
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: