522416 - Tab Previews must not do sync http requests

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect, P1)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

This made the problem in bug 521668 much much easier to hit, but that's a separate thing.  Regardless, tab preview code must never do sync network requests;  WindowsPreviewPerTab.jsm's _imageFromURI needs to become async.

Comment 1

[Justin Dolske [:Dolske]]

Use nsIFaviconService?

Comment 2

[Rob Arnold [:robarnold]]

That would probably be better. For some reason I thought that I was only getting favicon:// urls which were resolved to some local location.

Comment 3

[Marco Bonardo [:mak]]

i think using moz-anno:favicon:uri should be the way to go

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Hm, it's not a matter of just changing the URI, right?  The code has to be async, regardless of what URI is being used.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Who can take this bug - it's blocking the beta.

Comment 6

[Marco Bonardo [:mak]]

moz-anno:favicon uses an async channel

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

I think Vlad meant that it needs to be read asynchronously, so the required fix is a little more than just pulling the information from a different channel - though that will certainly help!

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Shawn said he'd take this

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: xpcom and netwerk changes v1.0

This was a lot harder than expected.  Step one - lets make a convenience method for JavaScript to easily open channels asynchronously.  Required a change in nsStorageStream.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: Taskbar changes v1.0

And the jsm changes needed with the new API.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: Taskbar changes v1.1

per comments from robarnold on irc

Comment 12

[Justin Dolske [:Dolske]]

So this is still reading from the network (albeit async now)? Seems like that's worth a followup bug to move this to use the favicon service (or Places' moz-anno stuff), to avoid the network requests entirely? [Along with the followup you mentioned for favicons often not loading at all.]

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> So this is still reading from the network (albeit async now)? Seems like that's
> worth a followup bug to move this to use the favicon service (or Places'
> moz-anno stuff), to avoid the network requests entirely? [Along with the
> followup you mentioned for favicons often not loading at all.]
The favicon service has no asynchronous way to read data from the database with the exception of using the moz-anno: protocol for favicons, which is what this does.  The moz-anno: protocol has to go through necko as a result, but it's not actually going over the network.  With these patches, this bit of code no longer does any synchronous IO on the main thread.

Comment 14

[Justin Dolske [:Dolske]]

(In reply to comment #13)
> With these patches, this bit of code no
> longer does any synchronous IO on the main thread.

Sure, but the point is a DB lookup should complete faster than an HTTP request, sync/async has nothing to do with it. We can change the favicon service to offer an async method if that's a win.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: Taskbar changes v1.2

Fix default icon race and a comment nit from robarnold.

Comment 16

[Marco Bonardo [:mak]]

(In reply to comment #14)
> We can change the favicon service to
> offer an async method if that's a win.

no reason, if you want to get an async channel from favicon service, as i said, and as i implemented apart, you can open an async channel to a moz-anno:favicon: address (Well now you could even use the netUtil method Shwan added). i tested that working, but Shawn's solution looks more polite than this http://mozilla.pastebin.com/m365fe0b4.

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> no reason, if you want to get an async channel from favicon service, as i said,
> and as i implemented apart, you can open an async channel to a
> moz-anno:favicon: address (Well now you could even use the netUtil method Shwan
> added). i tested that working, but Shawn's solution looks more polite than this
> http://mozilla.pastebin.com/m365fe0b4.
we also have to worry about a delayed load with the favicon service, right?
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsFaviconService.cpp#503

Comment 18

[Boris Zbarsky [:bzbarsky]]

A few quick questions:

1)  Is there a reason to use a storage stream here instead of a pipe?  The latter would allow data to be discarded as it's read, which might be desirable.

2)  If you use nsISimpleStreamListener you don't have to reimplement it in js (or rather just need to implement an nsIRequestObserver with an OnStopRequest that calles the callback), right?  What are the drawbacks?

Comment 19

[Rob Arnold [:robarnold]]

Attachment: Widget and test fixes v1.0

Ran the regression tests on Shawn's patch. Found a crash-near-startup bug (see widget fixes; test included to catch a regression on it) and all the browser tests for AeroPeek were broken in bug 520724 so those are fixed too.

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> 1)  Is there a reason to use a storage stream here instead of a pipe?  The
> latter would allow data to be discarded as it's read, which might be desirable.
biesi and I were talking on irc and thought that this would be simpler than using a pipe.  I don't fully recall the reasoning (we looked at three or four options all together).  For a more general case, a pipe might be better.

> 2)  If you use nsISimpleStreamListener you don't have to reimplement it in js
> (or rather just need to implement an nsIRequestObserver with an OnStopRequest
> that calles the callback), right?  What are the drawbacks?
Do I just pass the nsISimpleStreamListener to asyncOpen on the channel, and then OnStopRequest, flush init it with the either the nsIStorageStream's output stream or a pipe's output stream?

Comment 21

[Marco Bonardo [:mak]]

(In reply to comment #17)
> (In reply to comment #16)
> we also have to worry about a delayed load with the favicon service, right?
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsFaviconService.cpp#503
that's why i added a second icon refresh after 5s. but yeah that's the main problem of the favicon service approach.

Comment 22

[Boris Zbarsky [:bzbarsky]]

OK.  No strong opinions on storage stream vs pipe, I guess, other than pipe already supporting writefrom and such.

> Do I just pass the nsISimpleStreamListener to asyncOpen on the channel, and
> then OnStopRequest, flush init it with the either the nsIStorageStream's output
> stream or a pipe's output stream?

In your asyncOpen, you create the nsISimpleStreamListener, init it with the output end of a pipe or storage stream and an observer of your choice that watches for onStopRequest, and pass the nsISimpleStreamListener to the channel's asyncOpen.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406568 [details] [diff] [review]
xpcom and netwerk changes v1.0

>+    return Write(data.get(), aCount, _bytesWritten);

s/aCount/data.Length()/, right?

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #23)
> s/aCount/data.Length()/, right?
Those should be the same, right?  I could add an assertion there I suppose...

Comment 25

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Maybe call asyncOpen something like asyncFetch?  "asyncOpen" implies just opening to me, whereas the important part of the API (and this particular usage of it) is that it will open and fully read from the stream, ensuring that reading from the input stream will never block.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Those should be the same, right? 

Not if the stream hit EOF before aCount bytes got read from it.  Which can happen, no problem.

Comment 27

[Shawn Wilsher :sdwilsh]

Attachment: netwerk changes v2.0

s/asyncOpen/asyncFetch/ per comments by vlad on irc
Use nsIPipe and nsISimpleStreamListener

Comment 28

[Shawn Wilsher :sdwilsh]

Attachment: Taskbar changes v1.3

Updated to use the new API name.  Ready to land.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Not if the stream hit EOF before aCount bytes got read from it.  Which can
> happen, no problem.
Ah.  That code has all been removed from the latest patch anyway since I no longer needed to make that change.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406744 [details] [diff] [review]
netwerk changes v2.0

Looks good.

Comment 31

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/d3ce844996ee
http://hg.mozilla.org/mozilla-central/rev/a9e961601e20
http://hg.mozilla.org/mozilla-central/rev/3124e42df5a6

Comment 32

[Shawn Wilsher :sdwilsh]

Attachment: netwerk changes v2.0 for 1.9.2

This didn't apply perfectly on branch - one minor conflict in the test file.  This does.

Comment 33

[Marco Bonardo [:mak]]

Shawn, do you think makes sense to file a bug to use the favicon service when we fail fetching through http? (offline mode or network errors)

Comment 34

[Shawn Wilsher :sdwilsh]

I think it makes sense to always use the favicon service, and we should file a bug for that.

Comment 35

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d6cce120cff7
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/56c0fa3e56ba
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/429e7f7ba34d

relbranch soon...

Comment 36

[Shawn Wilsher :sdwilsh]

release branch!  I had to take the changesets for bug 521216 since it would require non-trivial changes for this work to apply to just the release branch.  That bug had already landed on 1.9.2, so I figure it's probably just fine.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c307c9951835
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5d6d66d2e92d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/28f2f49b61fe

Comment 37

[Marco Bonardo [:mak]]

i have filed Bug 522855 to implement this through the favicon service.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Awesome, Shawn. Thanks so much for getting this taken care of on 1.9.2 and the releasebranch.

Comment 39

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ugh... with today's nightly (Minefield, haven't tried Namaroka), I see deadlocks that look like this:

ntdll_77330000!NtWaitForSingleObject
KERNELBASE!WaitForSingleObjectEx
KERNEL32!WaitForSingleObjectExImplementation
KERNEL32!WaitForSingleObject
nspr4!_PR_MD_WAIT_CV
nspr4!_PR_WaitCondVar
nspr4!PR_Wait
xul!nsAutoMonitor::Wait
xul!nsPipeInputStream::Wait
xul!nsPipeInputStream::ReadSegments
xul!NS_InputStreamIsBuffered
xul!imgTools::DecodeImageData
xul!NS_InvokeByIndex_P
xul!XPCWrappedNative::CallMethod
xul!XPCWrappedNative::GetLock
xul!XPC_WN_OnlyIWrite_PropertyStub
MOZCRT19!arena_malloc_small
MOZCRT19!malloc
mozjs!JS_HashTableRawAdd
mozjs!JS_HashTableAdd

There are only two users of DecodeImageData, and the other one is in C++ code; so it has to be the one here.   This stack is bogus after the XPCWrappedNative bits due to xpconnect, but the top part is telling me that the PipeInputStream is blocking on DecodeImageData -- I'm guessing that DecodeImageData consumed all the data, but the pipe somehow didn't get notified that the base stream got closed.  So, it's getting a wouldblock and is calling Wait(), leading to badness.

Is a simple fix here to set the pipe to non-blocking via SetNonBlocking?  Or is there some deeper problem?

Comment 40

[Brian Carpenter [:geeknik]]

Is it possible that these deadlocks are behind the random 'not responding' issues I am having with the 17 October build of Minefield?

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I just hit this again despite having the taskbar previews pref set to false -- does that pref actually work?  It does seem to turn off the previews, but this code still seems to be executed.

(In reply to comment #40)
> Is it possible that these deadlocks are behind the random 'not responding'
> issues I am having with the 17 October build of Minefield?

If you're running Windows 7, then quite possibly, yes.

Comment 42

[Jim Jeffery not reading bug-mail 1/2/11]

I can confirm that the build with cset:
http://hg.mozilla.org/mozilla-central/rev/8976e1704153 is OK, no hangs on Acid3 test

The first build with cset: 
http://hg.mozilla.org/mozilla-central/rev/3124e42df5a6 which contains this patch locks hard, and have to kill the process. 

Win7RC build 7100 
AMD Athlon CPU Phenom II Quad X4, 8 gig RAM HD3200 onboard video 512meg

Comment 43

[Rob Arnold [:robarnold]]

(In reply to comment #41)
> I just hit this again despite having the taskbar previews pref set to false --
> does that pref actually work?  It does seem to turn off the previews, but this
> code still seems to be executed.

Yes, the previews' visibility is just set to false. That's the only difference.

Comment 44

[Sylvain Pasche]

Apparently, pages with empty favicons trigger the hang:

data:text/html,<link rel="icon" href="data:image/x-icon,">

Comment 45

[Shawn Wilsher :sdwilsh]

(In reply to comment #39)
> Is a simple fix here to set the pipe to non-blocking via SetNonBlocking?  Or is
> there some deeper problem?
hmm.  bz - did we want this line:
pipe.init(false, false, 0, PR_UINT32_MAX, null);
to be this instead:
pipe.init(true, true, 0, PR_UINT32_MAX, null);

Comment 46

[Boris Zbarsky [:bzbarsky]]

Er... we want non-blocking streams, so yes.  I double-checked that when I read the patch originally, and clearly got confused by all the negatives...

That said, would just calling close() on the output end of the pipe in the onStopRequest there before calling the callback address the issue?

Comment 47

[Boris Zbarsky [:bzbarsky]]

Ah, so the stack in comment 39 makes sense: we're trying to read 1 byte from a blocking stream.

I think we might want to both close() and make the streams non-blocking to be safe, though the former should be enough (changes the return from GetReadSegment from NS_BASE_STREAM_WOULD_BLOCK to NS_BASE_STREAM_CLOSED when the input end of the pipe is empty).

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Vlad, I'm resetting this as status1.9.2:--- on the assumption that you meant to have this continue to block the beta. (If that's not right, we should close this bug again and file a follow up.)

Comment 49

[Shawn Wilsher :sdwilsh]

(In reply to comment #48)
> Vlad, I'm resetting this as status1.9.2:--- on the assumption that you meant to
> have this continue to block the beta. (If that's not right, we should close
> this bug again and file a follow up.)
It needs to continue to block the beta.  I need to head out to dinner, but I'll make a patch tonight for bz to review.  I'll try to land it tomorrow.

bz - ideas for a test that would fail and won't in the "correct" situation?  Preferably xpcshell.

Comment 50

[Shawn Wilsher :sdwilsh]

Attachment: hang fix v1.0

I'll make a test for this on Monday when I get back to the office.  I'm having issues with building on this machine.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 406902 [details] [diff] [review]
hang fix v1.0

r=bzbarsky

If we're talking unit test for the fetch method here, you could fetch "data:text/plain," then in the caller try to read one byte from the input stream returned.  If it hangs, the test failed.  ;)

Comment 52

[Boris Zbarsky [:bzbarsky]]

Oh, and presumably do the read in a try/catch, since it'll throw BASE_STREAM_CLOSED when passing.

Comment 53

[Rob Arnold [:robarnold]]

This morning I foolishly updated to today's nightly and immediately encountered this hang on startup. I can now verify that this patch fixes the hang and the favicons still do show up as expected in the tab previews.

Comment 54

[Brian Carpenter [:geeknik]]

It would seem that a temporary workaround would be to toggle browser.chrome.favicons to false.  After I did this, I was able to get the Acid3 test to run w/o hanging.

Comment 55

[Shawn Wilsher :sdwilsh]

If somebody has the ability to land this, please do.  I'm not going to be able to do so for several hours.

As stated in comment 50, I'll write the test on Monday.  Comment 53 indicates that the patch fixes the hang.

Comment 56

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in:

http://hg.mozilla.org/mozilla-central/rev/001a1805c476
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5336e6da58ab

Comment 57

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

And on the relbranch:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/984d02abebf0

Comment 58

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #35)
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d6cce120cff7
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/56c0fa3e56ba
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/429e7f7ba34d

xpcshell tests have been hanging mozilla-1.9.2 since this set of changes landed there, and the hang fix has not changed that. Eg http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1255907641.1255910818.28959.gz
It passes xpcshell/tests/test_intl_strres/unit/test_bug397093.js and then hangs. xpcshell/tests/test_necko/unit/test_NetUtil.js is the next test in logs when it there isn't a hang.

The regression range is only the three changes above, which points the finger at d6cce120cff7. Could someone please check asap if this is a app regression or a test regression.

Comment 59

[Shawn Wilsher :sdwilsh]

I'm not sure how that's passing on mozilla-central either...

There should be a run_next_test() call here I think.  
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d6cce120cff7#l2.76

I'm in the process of rebuilding to test this fix, but I suspect it's it.

Comment 60

[Brian Carpenter [:geeknik]]

I just downloaded the latest hourly with this patch in it and when I toggle browser.chrome.favicons back to true, I don't get a hang on the Acid3 site or other sites that were hanging.

Comment 62

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Pushed the run_next_test() call to all 3 branches.

Comment 63

[Shawn Wilsher :sdwilsh]

(In reply to comment #62)
> Pushed the run_next_test() call to all 3 branches.
I presume that worked then?  My build just finally finished...

Comment 64

[Nick Thomas [:nthomas] (UTC+12)]

Apparently it didn't: http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox3.6-Unittest&errorparser=unittest&logfile=1255917030.1255919856.28064.gz&buildtime=1255917030&buildname=OS%20X%2010.5.2%20mozilla-1.9.2%20test%20everythingelse&fulltext=1
is http://hg.mozilla.org/releases/mozilla-1.9.2/rev/35804d6d3262

TEST-PASS | /builds/slave/mozilla-1.9.2-macosx-unittest-everythingelse/build/xpcshell/tests/test_intl_strres/unit/test_bug397093.js | test passed

command timed out: 1200 seconds without output, killing pid 372

Comment 65

[Shawn Wilsher :sdwilsh]

Silly.  NetUtil.ioService doesn't exist on 1.9.2, so the test fails there.  We never report the error because it's outside of the normal run_test() function (asynchronous testing FTL I guess).

Fixed in:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8217722e6af4
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/5879c8980e69

Comment 66

[Mike Beltzner [:beltzner, not reading bugmail]]

So is this now resolved? Looks like the patch fixes the issue (comment 60) and the test failures were resolved (comment 65).

Comment 67

[Mike Beltzner [:beltzner, not reading bugmail]]

Marking FIXED and status1.9.2beta1-fixed.

Comment 68

[Shawn Wilsher :sdwilsh]

(In reply to comment #66)
> So is this now resolved? Looks like the patch fixes the issue (comment 60) and
> the test failures were resolved (comment 65).
Left it open because I still need to write a test as stated in comment 50.  If we close it, I'm more likely to lose track of it.

Comment 69

[Shawn Wilsher :sdwilsh]

(In reply to comment #59)
> I'm not sure how that's passing on mozilla-central either...
> 
> There should be a run_next_test() call here I think.  
> http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d6cce120cff7#l2.76
Ugh - this was actually wrong.  The call that stops the server takes a callback, where we pass that method in anyway.  My new patch with the test will fix this.

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

This is a P1 blocker, so if you think we should resolve the test issue before we ship the beta, let's leave it open. If not, though, I'd suggest filing a follow-up for the tests, mark it blocking? and I'll + it as a P2 blocker.

Comment 71

[Shawn Wilsher :sdwilsh]

Attachment: tests for hang vix v1.0

The tests, as promised in comment 50

Comment 72

[Shawn Wilsher :sdwilsh]

I'll land on branch in a bit.  Patch doesn't apply completely cleanly, so being extra careful this time.

http://hg.mozilla.org/mozilla-central/rev/3314dc670ff4

Comment 73

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 407080 [details] [diff] [review]
tests for hang vix v1.0

r+, fwiw.

Comment 74

[Shawn Wilsher :sdwilsh]

Tests on branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/826d70847afc (release branch)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1fb7fbbb6c36

Comment 75

[Shawn Wilsher :sdwilsh]

And documentation:
https://developer.mozilla.org/en/JavaScript_code_modules/NetUtil.jsm#asyncFetch

Leaving dev-doc-needed so sheppy can look it over.

Comment 76

[Eric Shepherd [:sheppy]]

Looks good - nice job!

Comment 77

[Marcia Knous [:marcia]]

Shawn: What is the easiest way for QA verify this bug? Am seeing some weirdness while testing the candidate builds.  Thanks.

Comment 78

[Shawn Wilsher :sdwilsh]

(In reply to comment #77)
> Shawn: What is the easiest way for QA verify this bug? Am seeing some weirdness
> while testing the candidate builds.  Thanks.
That's probably best identified by vlad since he noticed the issue in the first place.  I can't think of an easy way to verify it.

Comment 79

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

"Doesn't crash during browsing" is really the only way I'm verifying it.  I was crashing trivially every 15 min or so with more than a handful of tabs open, especially upon session restore.  Bugzilla tabs in particular seemed highly likely to trigger the problem.

Comment 80

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

FWIW, I've been browsing and reviewing bugs with yesterday's nightly all day yesterday and today with no crashes; I'd be comfortable calling this VERIFIED based on that.