Closed Bug 965362 Opened 6 years ago Closed 6 years ago

Intermittent devtools/sourceeditor/test/browser_css_statemachine.js | Exception thrown at chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_statemachine.js:95 - SyntaxError: syntax error

Categories

(Core :: XPCOM, defect, P1)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: emorley, Assigned: ehsan)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Windows XP 32-bit mozilla-inbound opt test mochitest-browser-chrome on 2014-01-29 07:04:29 PST for push 192c0bc86b01

slave: t-xp32-ix-101

https://tbpl.mozilla.org/php/getParsedLog.php?id=33745195&tree=Mozilla-Inbound

{
07:40:03     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_autocompletion.js | Test 15 passed.
07:40:03     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_autocompletion.js | Test 16 passed.
07:40:03     INFO -  TEST-INFO | MEMORY STAT vsize after test: 754053120
07:40:03     INFO -  TEST-INFO | MEMORY STAT vsizeMaxContiguous after test: 428605440
07:40:03     INFO -  TEST-INFO | MEMORY STAT residentFast after test: 477601792
07:40:03     INFO -  TEST-INFO | MEMORY STAT heapAllocated after test: 160316542
07:40:03     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_autocompletion.js | finished in 572ms
07:40:03     INFO -  TEST-INFO | checking window state
07:40:03     INFO -  TEST-START | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_statemachine.js
07:40:03  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_statemachine.js | Exception thrown at chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_statemachine.js:95 - SyntaxError: syntax error
07:40:04     INFO -  TEST-INFO | MEMORY STAT vsize after test: 754053120
07:40:04     INFO -  TEST-INFO | MEMORY STAT vsizeMaxContiguous after test: 428605440
07:40:04     INFO -  TEST-INFO | MEMORY STAT residentFast after test: 477605888
07:40:04     INFO -  TEST-INFO | MEMORY STAT heapAllocated after test: 160516366
07:40:04     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/browser_css_statemachine.js | finished in 637ms
}

Line 95 is:
https://hg.mozilla.org/integration/mozilla-inbound/file/192c0bc86b01/browser/devtools/sourceeditor/test/browser_css_statemachine.js#l95

    95   for (let test of tests) {
    96     progress.dataset.progress = ++i;
    97     progressDiv.style.width = 100*i/tests.length + "%";
    98     completer.resolveState(limit(source, test[0]),
    99                            {line: test[0][0], ch: test[0][1]});
   100     if (checkState(test[1])) {
   101       ok(true, "Test " + i + " passed. ");
   102     }
   103     else {
   104       ok(false, "Test " + i + " failed. Expected state : [" + test[1] + "] but" +
   105          " found [" + completer.state + ", " + completer.selectorState + ", " +
   106          completer.completing + ", " +
   107          (completer.propertyName || completer.selector) + "].");
   108       progress.classList.add("failed");
   109     }
   110   }
hmm, read method is failing to read the file. Seems like it.
Priority: -- → P1
Optimizer, are you looking into this?
not atm. The frequency seems too low. Will add some more logging and see.
I think this is an actual XPCOM bug.  See for example this innocent looking code where we try to read all of the available bytes from the stream: <http://mxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/head.js#77>.  This is the utility read() function used in this test which intermittently fails.

But the implementation of nsScriptableInputStream::Read() is buggy: <http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsScriptableInputStream.cpp#44>.  It first decides on the amount of bytes that it wants to read, and then calls nsIInputStream::Read to read that amount, but it doesn't take into account that the fact that nsIInputStream::Read may be able to read fewer than the number of bytes requested.  The corresponding nsScriptableInputStream::ReadBytes function gets this right.

I have a patch which fixes this by refactoring the reading algorithm in a helper method and calling it from both of these places.
Component: Developer Tools: Source Editor → XPCOM
Product: Firefox → Core
Assignee: nobody → ehsan
Attachment #8395289 - Flags: review?(benjamin)
A try push with the patch I attached here revealed that this test still fails: <https://tbpl.mozilla.org/?tree=Try&rev=15e0bfb66789> which made me think more, and I realized that the read() function implemented here is actually buggy.  The read function creates an nsISyncStreamListener which spins the event loop waiting for more data every time that either Available() or Read() are called on its stream.  Because the read() function here assumes that all of the response of the HTTP request is delivered to the main thread in time for the call to Read() to be able to read everything and return, which is not necessarily the case.  This idiom seems to be repeated in a couple of more places in the devtools tests.  I have a patch to fix this by simply looping while there is more data available, which seems to fix this test failure when applied on top of the part 1 patch already posted: <https://tbpl.mozilla.org/?tree=Try&rev=2fbe60010d42>.

One of the other places mentioned above is browser_styleeditor_sourcemap_watching.js, for which bug 966805 is on file.  I'm marking this as a dependency but I don't know for a fact whether this patch will fix that bug yet.  We'll see.
Blocks: 966805
Attachment #8395478 - Flags: review?(rcampbell)
Comment on attachment 8395478 [details] [diff] [review]
Part 2: Do not assume that all of the contents of the HTTP channel will be available by the time we decide to read it synchronously in devtools tests; r=robcee

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

cool. thanks for this.
Attachment #8395478 - Flags: review?(rcampbell) → review+
Attachment #8395289 - Flags: review?(benjamin) → review+
I am guessing wrong bug number of the push.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Boo. I must be drunk when reopening.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Ehsan, Aurora and Beta are still affected. Since this is not just a test change, are the changes in /xpcom/io/* okay to be uplifted to beta and aurora ?
Flags: needinfo?(ehsan)
(In reply to Girish Sharma [:Optimizer] from comment #145)
> Ehsan, Aurora and Beta are still affected. Since this is not just a test
> change, are the changes in /xpcom/io/* okay to be uplifted to beta and
> aurora ?

No, I don't think so.  But the test only change should be fine, and it should help resolve at least part of these oranges.
Flags: needinfo?(ehsan)
Sheriffs, please uplift only attachment 8395478 [details] [diff] [review] to aurora and beta.
You need to log in before you can comment on or make changes to this bug.