Closed Bug 1524359 Opened 5 years ago Closed 5 years ago

49,600 instances of "NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005" emitted from docshell/shistory/nsSHistory.cpp during linux64 debug testing

Categories

(Core :: DOM: Navigation, defect)

Product:
Core
Component:
DOM: Navigation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox68 --- fixed

People

(Reporter: erahm, Assigned: f20160385)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

bug 1524359 - 49,600 instances of "NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005" emitted from docshell/shistory/nsSHistory.cpp during linux64 debug testing
47 bytes, text/x-phabricator-request
Details | Review

49587 WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005: file docshell/shistory/nsSHistory.cpp, line 1198

This warning [1] shows up in the following test suites:

  2926 - test-linux64/debug-mochitest-plain-headless-e10s-9 h9
  2882 - test-linux64/debug-web-platform-tests-sw-e10s-9 wpt9
  2882 - test-linux64/debug-web-platform-tests-e10s-9 wpt9
  1872 - test-linux64/debug-mochitest-e10s-9 9
  1872 - test-linux64/debug-mochitest-plain-headless-sw-e10s-9 h9
  1871 - test-linux64/debug-mochitest-sw-e10s-9 9
  1410 - test-linux64/debug-mochitest-e10s-8 8
  1410 - test-linux64/debug-mochitest-plain-headless-sw-e10s-8 h8
  1410 - test-linux64/debug-mochitest-sw-e10s-8 8
  1089 - test-linux64/debug-web-platform-tests-e10s-14 wpt14
  1089 - test-linux64/debug-web-platform-tests-sw-e10s-14 wpt14
   772 - test-linux64/debug-mochitest-plain-headless-sw-e10s-13 h13
   772 - test-linux64/debug-mochitest-sw-e10s-13 13
   772 - test-linux64/debug-mochitest-plain-headless-e10s-13 h13
   772 - test-linux64/debug-mochitest-e10s-13 13
   758 - test-linux64/debug-mochitest-browser-chrome-e10s-7 bc7
   689 - test-linux64/debug-web-platform-tests-sw-e10s-7 wpt7
   689 - test-linux64/debug-web-platform-tests-e10s-7 wpt7
   689 - test-linux64/debug-web-platform-tests-sw-e10s-17 wpt17
   689 - test-linux64/debug-web-platform-tests-e10s-17 wpt17
   591 - test-linux64/debug-web-platform-tests-sw-e10s-4 wpt4
   591 - test-linux64/debug-web-platform-tests-e10s-4 wpt4
   587 - test-linux64/debug-web-platform-tests-sw-e10s-6 wpt6
   587 - test-linux64/debug-web-platform-tests-e10s-6 wpt6
   558 - test-linux64/debug-web-platform-tests-e10s-1 wpt1
   558 - test-linux64/debug-web-platform-tests-sw-e10s-1 wpt1
   555 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-11 bc11
   517 - test-linux64/debug-web-platform-tests-sw-e10s-18 wpt18
   517 - test-linux64/debug-web-platform-tests-e10s-18 wpt18
   421 - test-linux64/debug-web-platform-tests-e10s-8 wpt8
   421 - test-linux64/debug-web-platform-tests-sw-e10s-8 wpt8
   417 - test-linux64/debug-mochitest-browser-chrome-e10s-5 bc5
   368 - test-linux64/debug-mochitest-plain-headless-e10s-2 h2
   368 - test-linux64/debug-mochitest-e10s-2 2
   368 - test-linux64/debug-mochitest-plain-headless-sw-e10s-2 h2
   368 - test-linux64/debug-mochitest-sw-e10s-2 2
   356 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-10 bc10
   356 - test-linux64/debug-mochitest-plain-headless-e10s-8 h8
   352 - test-linux64/debug-mochitest-media-e10s-2 mda2
   352 - test-linux64/debug-mochitest-media-sw-e10s-2 mda2
   340 - test-linux64/debug-mochitest-media-sw-e10s-1 mda1
   340 - test-linux64/debug-mochitest-media-e10s-1 mda1
   338 - test-linux64/debug-web-platform-tests-e10s-2 wpt2
   338 - test-linux64/debug-web-platform-tests-sw-e10s-2 wpt2
   298 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-1 bc1
   290 - test-linux64/debug-mochitest-browser-chrome-e10s-2 bc2
   270 - test-linux64/debug-mochitest-browser-chrome-e10s-4 bc4
   268 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-2 bc2
   257 - test-linux64/debug-web-platform-tests-e10s-10 wpt10
   257 - test-linux64/debug-web-platform-tests-sw-e10s-10 wpt10
   257 - test-linux64/debug-web-platform-tests-sw-e10s-13 wpt13
   257 - test-linux64/debug-web-platform-tests-e10s-13 wpt13
   254 - test-linux64/debug-web-platform-tests-sw-e10s-12 wpt12
   254 - test-linux64/debug-web-platform-tests-e10s-12 wpt12
   250 - test-linux64/debug-web-platform-tests-e10s-11 wpt11
   250 - test-linux64/debug-web-platform-tests-sw-e10s-11 wpt11
   250 - test-linux64/debug-web-platform-tests-sw-e10s-16 wpt16
   250 - test-linux64/debug-web-platform-tests-e10s-16 wpt16
   232 - test-linux64/debug-mochitest-e10s-7 7
   226 - test-linux64/debug-mochitest-chrome-sw-1 c1
   226 - test-linux64/debug-web-platform-tests-sw-e10s-15 wpt15
   226 - test-linux64/debug-mochitest-chrome-1 c1
   226 - test-linux64/debug-web-platform-tests-e10s-15 wpt15
   221 - test-linux64/debug-mochitest-plain-headless-e10s-7 h7
   216 - test-linux64/debug-crashtest-e10s C
   211 - test-linux64/debug-crashtest-sw-e10s C
   207 - test-linux64/debug-web-platform-tests-sw-e10s-5 wpt5
   207 - test-linux64/debug-web-platform-tests-e10s-5 wpt5
   205 - test-linux64/debug-mochitest-chrome-2 c2
   205 - test-linux64/debug-mochitest-chrome-sw-2 c2
   183 - test-linux64/debug-web-platform-tests-sw-e10s-3 wpt3
   183 - test-linux64/debug-web-platform-tests-e10s-3 wpt3
   166 - test-linux64/debug-mochitest-chrome-sw-3 c3
   166 - test-linux64/debug-mochitest-chrome-3 c3
   152 - test-linux64/debug-mochitest-plain-headless-sw-e10s-5 h5
   152 - test-linux64/debug-mochitest-sw-e10s-5 5
   152 - test-linux64/debug-mochitest-e10s-5 5
   152 - test-linux64/debug-mochitest-plain-headless-e10s-5 h5
   132 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-7 bc7
   119 - test-linux64/debug-mochitest-browser-chrome-e10s-1 bc1
    98 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-12 bc12
    98 - test-linux64/debug-mochitest-browser-chrome-e10s-12 bc12
    91 - test-linux64/debug-mochitest-clipboard-sw-e10s cl
    91 - test-linux64/debug-mochitest-clipboard-e10s cl
    90 - test-linux64/debug-mochitest-e10s-3 3
    90 - test-linux64/debug-mochitest-sw-e10s-3 3
    90 - test-linux64/debug-mochitest-plain-headless-e10s-3 h3
    88 - test-linux64/debug-mochitest-plain-headless-sw-e10s-3 h3
    83 - test-linux64/debug-mochitest-sw-e10s-15 15
    82 - test-linux64/debug-mochitest-devtools-chrome-sw-e10s-6 dt6
    82 - test-linux64/debug-mochitest-devtools-chrome-e10s-1 dt1
    71 - test-linux64/debug-mochitest-plain-headless-e10s-15 h15
    71 - test-linux64/debug-mochitest-plain-headless-sw-e10s-15 h15
    70 - test-linux64/debug-mochitest-sw-e10s-7 7
    69 - test-linux64/debug-mochitest-media-sw-e10s-3 mda3
    69 - test-linux64/debug-mochitest-media-e10s-3 mda3
    68 - test-linux64/debug-mochitest-plain-headless-e10s-16 h16
    68 - test-linux64/debug-mochitest-e10s-16 16
    68 - test-linux64/debug-mochitest-plain-headless-sw-e10s-16 h16
    62 - test-linux64/debug-mochitest-sw-e10s-12 12
    62 - test-linux64/debug-mochitest-e10s-12 12
    62 - test-linux64/debug-mochitest-plain-headless-sw-e10s-12 h12
    62 - test-linux64/debug-mochitest-plain-headless-e10s-12 h12
    60 - test-linux64/debug-mochitest-e10s-11 11
    60 - test-linux64/debug-mochitest-plain-headless-e10s-11 h11
    60 - test-linux64/debug-mochitest-plain-headless-sw-e10s-11 h11
    60 - test-linux64/debug-mochitest-sw-e10s-11 11
    59 - test-linux64/debug-mochitest-plain-headless-sw-e10s-7 h7
    57 - test-linux64/debug-mochitest-e10s-15 15
    48 - test-linux64/debug-test-verify-e10s-1 TV1
    47 - test-linux64/debug-mochitest-sw-e10s-14 14
    47 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-3 bc3
    47 - test-linux64/debug-mochitest-e10s-14 14
    46 - test-linux64/debug-mochitest-e10s-6 6
    46 - test-linux64/debug-mochitest-plain-headless-e10s-6 h6
    43 - test-linux64/debug-mochitest-browser-chrome-e10s-9 bc9
    42 - test-linux64/debug-mochitest-sw-e10s-16 16
    39 - test-linux64/debug-mochitest-sw-e10s-10 10
    39 - test-linux64/debug-mochitest-plain-headless-sw-e10s-10 h10
    39 - test-linux64/debug-mochitest-plain-headless-e10s-10 h10
    39 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-8 bc8
    39 - test-linux64/debug-mochitest-e10s-10 10
    34 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-6 bc6
    34 - test-linux64/debug-mochitest-plain-headless-sw-e10s-6 h6
    34 - test-linux64/debug-mochitest-sw-e10s-6 6
    33 - test-linux64/debug-mochitest-plain-headless-e10s-14 h14
    33 - test-linux64/debug-mochitest-plain-headless-sw-e10s-14 h14
    32 - test-linux64/debug-web-platform-tests-reftests-e10s-5 Wr5
    32 - test-linux64/debug-web-platform-tests-wdspec-e10s-1 Wd1
    32 - test-linux64/debug-web-platform-tests-wdspec-sw-e10s-1 Wd1
    32 - test-linux64/debug-web-platform-tests-reftests-e10s-1 Wr1
    32 - test-linux64/debug-web-platform-tests-reftests-sw-e10s-5 Wr5
    32 - test-linux64/debug-web-platform-tests-reftests-sw-e10s-1 Wr1
    30 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-15 bc15
    30 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-16 bc16
    30 - test-linux64/debug-mochitest-plain-headless-sw-e10s-4 h4
    30 - test-linux64/debug-mochitest-browser-chrome-e10s-16 bc16
    30 - test-linux64/debug-mochitest-browser-chrome-e10s-15 bc15
    28 - test-linux64/debug-mochitest-sw-e10s-4 4
    28 - test-linux64/debug-mochitest-plain-headless-e10s-4 h4
    28 - test-linux64/debug-mochitest-e10s-4 4
    24 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-5 bc5
    24 - test-linux64/debug-mochitest-a11y a11y
    24 - test-linux64/debug-mochitest-a11y-sw a11y
    21 - test-linux64/debug-mochitest-browser-chrome-e10s-14 bc14
    21 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-4 bc4
    21 - test-linux64/debug-mochitest-browser-chrome-e10s-11 bc11
    21 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-14 bc14
    21 - test-linux64/debug-mochitest-browser-chrome-e10s-13 bc13
    21 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-13 bc13
    20 - test-linux64/debug-web-platform-tests-wdspec-sw-e10s-2 Wd2
    20 - test-linux64/debug-marionette-headless-e10s MnH
    20 - test-linux64/debug-marionette-e10s Mn
    20 - test-linux64/debug-web-platform-tests-wdspec-e10s-2 Wd2
    18 - test-linux64/debug-mochitest-browser-chrome-e10s-6 bc6
    15 - test-linux64/debug-web-platform-tests-reftests-sw-e10s-2 Wr2
    15 - test-linux64/debug-web-platform-tests-reftests-e10s-2 Wr2
    12 - test-linux64/debug-mochitest-devtools-chrome-sw-e10s-8 dt8
    12 - test-linux64/debug-mochitest-devtools-chrome-e10s-8 dt8
    11 - test-linux64/debug-mochitest-browser-chrome-e10s-10 bc10
    11 - test-linux64/debug-mochitest-browser-chrome-e10s-8 bc8
    10 - test-linux64/debug-mochitest-devtools-chrome-sw-e10s-9 dt9
    10 - test-linux64/debug-mochitest-devtools-chrome-e10s-9 dt9
    10 - test-linux64/debug-web-platform-tests-reftests-sw-e10s-6 Wr6
    10 - test-linux64/debug-web-platform-tests-reftests-e10s-6 Wr6
     9 - test-linux64/debug-firefox-ui-functional-remote-e10s en-US
     8 - test-linux64/debug-reftest-e10s-8 R8
     8 - test-linux64/debug-reftest-no-accel-sw-e10s-8 Ru8
     8 - test-linux64/debug-reftest-no-accel-e10s-8 Ru8
     8 - test-linux64/debug-reftest-sw-e10s-8 R8
     6 - test-linux64/debug-mochitest-browser-chrome-e10s-3 bc3
     6 - test-linux64/debug-mochitest-gpu-sw-e10s gpu
     6 - test-linux64/debug-web-platform-tests-reftests-e10s-4 Wr4
     6 - test-linux64/debug-web-platform-tests-reftests-sw-e10s-4 Wr4
     6 - test-linux64/debug-mochitest-devtools-chrome-sw-e10s-5 dt5
     6 - test-linux64/debug-mochitest-devtools-chrome-e10s-7 dt7
     6 - test-linux64/debug-mochitest-gpu-e10s gpu
     5 - test-linux64/debug-reftest-no-accel-sw-e10s-6 Ru6
     5 - test-linux64/debug-reftest-e10s-6 R6
     5 - test-linux64/debug-reftest-no-accel-e10s-6 Ru6
     5 - test-linux64/debug-reftest-sw-e10s-6 R6
     4 - test-linux64/debug-mochitest-sw-e10s-1 1
     4 - test-linux64/debug-web-platform-tests-reftests-sw-e10s-3 Wr3
     4 - test-linux64/debug-reftest-e10s-5 R5
     4 - test-linux64/debug-mochitest-plain-headless-e10s-1 h1
     4 - test-linux64/debug-reftest-sw-e10s-5 R5
     4 - test-linux64/debug-reftest-no-accel-sw-e10s-3 Ru3
     4 - test-linux64/debug-reftest-sw-e10s-3 R3
     4 - test-linux64/debug-reftest-no-accel-e10s-5 Ru5
     4 - test-linux64/debug-reftest-e10s-3 R3
     4 - test-linux64/debug-mochitest-browser-chrome-sw-e10s-9 bc9
     4 - test-linux64/debug-mochitest-plain-headless-sw-e10s-1 h1
     4 - test-linux64/debug-reftest-no-accel-sw-e10s-5 Ru5
     4 - test-linux64/debug-mochitest-e10s-1 1
     4 - test-linux64/debug-web-platform-tests-reftests-e10s-3 Wr3
     4 - test-linux64/debug-reftest-no-accel-e10s-3 Ru3
     2 - test-linux64/debug-firefox-ui-functional-local-e10s en-US
     2 - test-linux64/debug-mochitest-devtools-chrome-sw-e10s-4 dt4
     2 - test-linux64/debug-mochitest-devtools-chrome-e10s-4 dt4
     2 - test-linux64/debug-mochitest-devtools-chrome-sw-e10s-2 dt2
     2 - test-linux64/debug-mochitest-devtools-chrome-e10s-6 dt6

It shows up in 5013 tests. A few of the most prevalent:

  2912 - [e10s] layout/base/tests/test_reftests_with_caret.html
  1280 - [e10s] dom/tests/mochitest/ajax/jquery/test_jQuery.html
   872 - [e10s] /dom/nodes/Document-characterSet-normalization.html
   864 - [e10s] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-auto.html
   864 - [e10s] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-object-fixed.html
   864 - [e10s] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-fixed.html
   864 - [e10s] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-object-percentage.html
   864 - [e10s] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-object-auto.html
   864 - [e10s] /html/rendering/replaced-elements/svg-embedded-sizing/svg-in-iframe-percentage.html
   672 - [e10s] /encoding/single-byte-decoder.html?document

[1] https://hg.mozilla.org/mozilla-central/annotate/9ee54a21a22a/docshell/shistory/nsSHistory.cpp#l1198

Nick, it looks like you added this NS_ENSURE in bug 1486358, do you think it's safe to convert to an if?

Blocks: 1486358
Flags: needinfo?(n.nethercote)

Hi, I want to work on this. Please check the patch submitted.

Flags: needinfo?(erahm)

Nika: I added two NS_ENSURE_SUCCESS calls in https://hg.mozilla.org/mozilla-central/rev/e39b65db5b40. I see now that this could change behaviour.

With the old code, if both GetEntryAtIndex() calls failed, they would both return nullptr and the subsequent IsSameTreeRoot() call would succeed and an element would be removed. With the new code we'll just do an early return. Do you have an opinion about which behaviour is preferable?

Flags: needinfo?(n.nethercote) → needinfo?(nika)

Forwarding ni? to :peterv who is deeper in the shistory code than me right now.

Flags: needinfo?(nika) → needinfo?(peterv)

I do think it's ok to skip the rest of the code in nsSHistory::RemoveDuplicate if both root1 and root2 are null, but I'm not entirely sure yet. Maybe smaug remembers, he wrote the original code: is it ok if nsSHistory::RemoveDuplicate doesn't do anything if the entry at aIndex and the next/previous entry are both null, or does it need to remove one of the nulls?

Flags: needinfo?(peterv) → needinfo?(bugs)

Hmm, actually, this isn't when they're both null, this is when aIndex and/or the next/previous index are out of range. That does seem more sketchy. I don't think we need to do anything in that case, since there really is no duplicate. So returning false would be the right thing, and not worthy of a warning.

I'm not familiar with this code anymore. All the SHTransaction code has disappeared and what not.

Flags: needinfo?(bugs)

Hmm, I don't think the part that we care about has fundamentally changed. But ok, we'll try to understand it I guess.

I think I still stand by comment 8 then.

Changing the two NS_ENSURE_SUCCESS calls to normal if statements sounds fine to me, then.

Khyati, can you update your patch to convert both of the calls?

Done. Please check it. Also, could you make it assigned to me? Thanks.

Flags: needinfo?(erahm) → needinfo?(n.nethercote)
Assignee: nobody → f20160385
Flags: needinfo?(n.nethercote)

peterv, do you have time to review this? I'm happy to rubber stamp it, given comment 8 I assume that's effectively an r+.

Flags: needinfo?(peterv)
Keywords: checkin-needed

Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e251a566abb6
49,600 instances of "NS_ENSURE_SUCCESS(rv, false) failed with result 0x80004005" emitted from docshell/shistory/nsSHistory.cpp during linux64 debug testing r=peterv

Keywords: checkin-needed

https://hg.mozilla.org/mozilla-central/rev/e251a566abb6

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: needinfo?(peterv)
You need to log in before you can comment on or make changes to this bug.