329410 - <select style="-moz-binding: url(#t)"> hangs the browser (iExploder test 10030070)

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Joonas]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060303
Firefox/1.6a1

<select style="-moz-binding: url(#t)"> hangs the browser (iExploder test 10030070)

(Filing as a new bug, see bug 324465 comment 7)

Found using http://toadstool.se/software/iexploder/

Comment 1

[Joonas]

Attachment: testcase

Test case, the same as bug 324465 attachment 214071 [details]

The source is <select style="-moz-binding: url(#t)">

Comment 2

[Boris Zbarsky [:bzbarsky]]

So...  nsXBLService::FetchBindingDocument forces a sync load if the bound element is an <html:select>.  This was added to fix the XBL form controls issue in bug 128408.

So we try to do a sync HTTP load of a URI we're in the middle of loading, and I bet we end up blocking on the cache in HTTP, leading to deadlock.  Darin, biesi, am I correct that that's what would happen?

I figure the right thing to do is to change the XBL form controls code to only happen when XBL form controls are enabled.  Or better yet, remove that special-case altogether; shouldn't it be covered by the IsResourceURI check we do here anyway?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: This fixes it for me

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

it's presumably waiting for the cache entry to become valid, which never happens, as the original load is blocked by this sync call.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Right.  That makes sense...

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 214140 [details] [diff] [review]
This fixes it for me

this might change some performance characteristics of startup, watch out for that, but looks ok.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fixed.  Ts seems to be ok.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.  Note that this is basically a deadlock hang, so I'm not sure we care enough to put this on branches...  sicking, bryner, what do you think?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The 1.8.1 maybe, other then that I'm not sure if it's worth it. a=me if you want to do that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8.1 branch patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Same, but remove the unused |ni| variable

I'll remove it on trunk too...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.1 branch.