Closed Bug 1435899 Opened 2 years ago Closed 1 year ago

Use nsStringStream for the data channel buffer

Categories

(Core :: Networking, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files)

As a follow up to bug 1333899 a further enhancement is to us nsStringStream for the buffer rather than a pipe. This ideally should avoid an additional buffer copy.

Nathan also mentioned in bug 1392231 that we might be able to use a yet-to-be-implemented streaming base64 decoder as well, but that wouldn't work for the non-base64 encoded case.
A pipe is no longer used for the input stream, instead we use a string stream
which in most cases will be able to share the string data buffer rather than
copying it.

This is the final piece from bug 1435899, we can't land it until bug 1371699
lands but we might as well review it now.
Attachment #8948552 - Flags: review?(bzbarsky)
Priority: -- → P5
Whiteboard: [necko-triaged]
Comment on attachment 8948552 [details] [diff] [review]
Use nsStringStream for the data channel buffer

r=me
Attachment #8948552 - Flags: review?(bzbarsky) → review+
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fa8c5b051ac
Use nsStringStream for the data channel buffer. r=bz
I can repro with:

./mach test image/test/mochitest/test_bug1325080.html

I get the feeling bug 1371699 didn't actually fix whatever was blocking us from landing this originally. baku, any chance you could take a look or give me some pointers?
Flags: needinfo?(erahm) → needinfo?(amarchesini)
Your code is correct. The issue here is just a timing problem. With your patch, we end up using NonBlockingAsyncInputStream and the async wait callback is executed 1 cycle after the current setup.
This is enough to make the test failing.

This happens because after a Read()/ReadSegments() that drains the underlying stream, the following ::Available() calls returns 0. An AsyncWait() is executed, and finally we communicate that the underlying stream is closed.

But we can immediately return NS_BASE_STREAM_CLOSED in ::Available() when the underlying stream->Available() returns 0, because nothing else can be read. This removes the extra cycle.

I'm pushing this patch to try together with the Eric's one.
Flags: needinfo?(amarchesini)
Attachment #8954299 - Flags: review?(nfroyd)
Right, see bug 1333899 comment 57.

Maybe if you do that for NonBlockingAsyncInputStream as opposed to stringstream itself you can avoid all the other fallout I ran into...
Oh, and I was about to suggest that we fix test_bug1325080.html to wait for image onload... but then I read the bug it's trying to test.  We should add a comment to that test about the fact that it is in fact testing next-tick behavior for data: images....
Comment on attachment 8954299 [details] [diff] [review]
NonBlockingAsyncInputStream closing underlying smtream sooner

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

I think this is OK?  Closing the sub-stream in response to Available seems somewhat sketchy, but see below.

::: xpcom/io/NonBlockingAsyncInputStream.cpp
@@ +175,5 @@
> +  }
> +
> +  // Nothing more to read. Let's close the stream now.
> +  if (*aLength == 0) {
> +    mInputStream->Close();

This is quite surprising to me that checking for the amount available would close the stream as a side effect.  The documentation for nsIInputStream::Available is not super-clear on this point, but I think this behavior is permitted.  mozilla::dom::cache::ReadStream::Inner::Available is the only other instance I could find of this behavior in the tree.

@@ +176,5 @@
> +
> +  // Nothing more to read. Let's close the stream now.
> +  if (*aLength == 0) {
> +    mInputStream->Close();
> +    mClosed = true;

I think you need to lock around this setting of `mClosed`.

Do we need to worry about all the extra work that NonBlockingAsyncInputStream::Close does and do some of that here?
Attachment #8954299 - Flags: review?(nfroyd) → review+
> I think this is OK?  Closing the sub-stream in response to Available seems
> somewhat sketchy, but see below.

Right. Note that, in theory, the current behavior is correct, but it introduces an extra cycle that makes the test unhappy.

> Do we need to worry about all the extra work that
> NonBlockingAsyncInputStream::Close does and do some of that here?

No, because the async wait callback used in Close() should be executed only ::Close() or ::CloseWithStatus() is called or if an error is executed.

Btw, we should get rid of WAIT_CLOSURE_ONLY. It's not used and it increases the level of complexity of AsyncWait().
froydnj, are you planning to land my patch together with yours? Or you want me to do it?
Flags: needinfo?(nfroyd)
I think the patch is Eric's?
Flags: needinfo?(nfroyd) → needinfo?(erahm)
Flags: needinfo?(erahm)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f22f1ab67c5a
Close underlying stream in NonBlockingAsyncInputStream sooner. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/925adb267211
Use nsStringStream for the data channel buffer. r=bz
Keywords: checkin-needed
(In reply to Andreea Pavel [:apavel] from comment #15)
> Backed out for failing android at 
> modules/libpref/test/unit/test_defaultValues.js
> 
> Push that started the failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=017dbfcb23698b9484a4467221e86a0dca980326
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=167058653&repo=mozilla-
> inbound
> 
> Backout:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 09ecb6d589490df1abb869bf025ce094ba87bcac

Looks like we're randomly segfaulting on android, Andreea is there a way to get the crashing stacks on android? Normally we'd see a stack dump in the log on desktop platforms.
Flags: needinfo?(erahm) → needinfo?(apavel)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #16)
> (In reply to Andreea Pavel [:apavel] from comment #15)
> > Backed out for failing android at 
> > modules/libpref/test/unit/test_defaultValues.js
> > 
> > Push that started the failures:
> > https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> > inbound&revision=017dbfcb23698b9484a4467221e86a0dca980326
> > 
> > Failure log:
> > https://treeherder.mozilla.org/logviewer.html#?job_id=167058653&repo=mozilla-
> > inbound
> > 
> > Backout:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 09ecb6d589490df1abb869bf025ce094ba87bcac
> 
> Looks like we're randomly segfaulting on android, Andreea is there a way to
> get the crashing stacks on android? Normally we'd see a stack dump in the
> log on desktop platforms.

Actually looking at the other tests (M, CPP) this is XPCJSContext::Initialize failing in NewXPCJSContext [1]:

> [task 2018-03-09T21:02:40.520Z] 21:02:40     INFO -   0  libxul.so!XPCJSContext::NewXPCJSContext [XPCJSContext.cpp:017dbfcb23698b9484a4467221e86a0dca980326 : 0 + 0x6]

I wonder if we just had a bad build/test instance.

[1] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/js/xpconnect/src/XPCJSContext.cpp#1214
Flags: needinfo?(apavel)
The warning [1]:

> 03-09 13:02:22.778   806   826 I Gecko   : [806, Main Thread] WARNING: 'NS_FAILED(rv)', file /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp, line 1019

So vaguely `CycleCollectedJSContext::Initialize` was failing, but we don't log the rv so that's about all we have to go on. It's possible the stream changes broke loading the startup cache, or it's possible this was just a bad test instance. I'm going to push a run to try.

[1] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/js/xpconnect/src/XPCJSContext.cpp#1022-1024
> is there a way to get the crashing stacks on android?

Please file a bug on this happening?  We should be getting stacks for crashes in logs.
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5461b62599d7
Close underlying stream in NonBlockingAsyncInputStream sooner. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4029bc621f5
Use nsStringStream for the data channel buffer. r=bz
https://hg.mozilla.org/mozilla-central/rev/5461b62599d7
https://hg.mozilla.org/mozilla-central/rev/c4029bc621f5
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Blocks: 1511692
You need to log in before you can comment on or make changes to this bug.