Closed Bug 965362 Opened 10 years ago Closed 10 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)

Product:
Core
Component:
XPCOM
Platform:
x86
Windows XP
Type:
defect

Tracking

()

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

People

(Reporter: emorley, Assigned: ehsan.akhgari)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Make sure that nsScriptableInputStream::Read reads all of the allowed available bytes from the input stream; r=bsmedberg
2.84 KB, patch
benjamin
: review+
Details | Diff | Splinter 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
2.83 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review 
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: 10 years ago → 10 years ago
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: