401743 - [FIX]ASSERTION: Shouldn't have pending bindings!: 'mAttachedStack.Length() == 0'

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: semi reduced testcase

ASSERTION: Shouldn't have pending bindings!: 'mAttachedStack.Length() == 0', file 1.9.0/mozilla/content/xbl/src/nsBindingManager.cpp, line 913

The testcase references an external css file that contains characters I can't save in my editor.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jonas, this looks like a case where EnsureScriptAPI is failing so we're bailing out of binding attachment processing altogether...  That doesn't seem like the right thing to do.

Comment 2

[Boris Zbarsky [:bzbarsky]]

In particular, a JS syntax error in an XBL property or method will cause EnsureScriptAPI to return NS_ERROR_FAILURE.  But that doesn't mean we want to abandon processing all the other bindings still in the queue.

Comment 3

[Jesse Ruderman]

Loading the attachment from Bugzilla doesn't trigger the assertion for me (on Mac).  Am I doing the right thing to attempt to reproduce the bug?

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: testcase without crap

Comment 5

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

I have this bug fixed locally, still working on the other regressions.

Comment 6

[Jesse Ruderman]

Hmm, I managed to reproduce the bug once (with the new testcase) but I don't see it every time I load the testcase.

Comment 7

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

Fixed by patch in bug 401463

Comment 8

[Bob Clary [:bc] (inactive)]

I still see the assert with a fresh dep debug build on linux with the attached testcase.

Comment 9

[Bill Gianopoulos [:WG9s]]

It probably has nothing to do with the issue here, but shouldn't the html in bug testcases attached to bugs actually validate?

Comment 10

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)

also with a clobber build.

(In reply to comment #9)

If it could be reproduced with valid markup, sure. But I don't think it is a requirement and I didn't try to make it valid. The important issue was a small (minimal?) testcase that demonstrates the issue. In many cases, valid markup wouldn't even reproduce the issue.

Comment 11

[Bill Gianopoulos [:WG9s]]

(In reply to comment #10)

> If it could be reproduced with valid markup, sure. But I don't think it is a
> requirement and I didn't try to make it valid. The important issue was a small
> (minimal?) testcase that demonstrates the issue. In many cases, valid markup
> wouldn't even reproduce the issue.
> 
Well, that was kind of my point.  If it can only be reproduced with invalid markup is it really a bug?  Although I guess it should not assert in any case.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #11)
> Well, that was kind of my point.  If it can only be reproduced with invalid
> markup is it really a bug?

Yes. Browsers need to deal with invalid markup.

Comment 13

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

So the reason this is happening still is that there's an XBL constructor that does a synchronous XHR request. Since the request is synchronous we'll pump the eventqueue, causing the mProcessAttachedQueueEvent to fire. But since we're still inside the XBL ctor, and thus inside ProcessAttachedQueue mProcessingAttachedStack is true and the new call to ProcessAttachedQueue will bail.

This rightfully causes an assertion as the event expects the mAttachedStack to get processed. The result is that the stack might never get fully processed.

This has nothing to do with bug 345711 though, it should have been a problem ever since bug 267833 though, so adjusting dependency.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> an XBL constructor that does a synchronous XHR request

Could we make that throw?  Pretty please?

Where's this binding coming from?  Would anyone actually care if we broke it?

> The result is that the stack might never get fully processed.

Of course the result of the sync XHR is that any bindings one tries to instantiate while it's pending are broken: their ctors won't fire when they should.  Better hope the user doesn't open a window before you server responds....  Which means that a site could use that to DOS the user, basically.  Which is why I think we should make it throw.  Of course posing modal dialogs from ctors has similar issues (and should likewise be prohibited, imo).

We could easily repost the binding instantiation event in the (rare) case that it fires during attached queue processing, right?  That will make this particular case sort of poll on the XHR, but somehow I don't care enough to optimize for this sort of thing.

Sound reasonable?

Comment 15

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

It's not really a problem if a content page is blocking ProcessAttachedQueue since the queue is per document.

The real problem here is IMHO that we're assuming that the posted runnable will run at a "safe" time, whereas in reality we in a number of places pump the event queue such that it is in fact not they will run only when it's semi-safe.

I wonder if we could add another flag-value to nsIThread::Dispatch stating that the runnable should only run in the top event queue. I would think we actually want that in more places.

Comment 16

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

Not marking this a blocker since it'll only break xbl ctors that does really whacky things. And I don't think it's harmful as far as being exploitable.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> It's not really a problem if a content page is blocking ProcessAttachedQueue
> since the queue is per document.

Ah, good point.

As for the rest... there is no "top event queue" usually.  There's just a single event queue that anyone can pump.  I suppose we could have a way of not processing certain runnables if another runnable is running right now...

Comment 18

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

Is there no concept of the top-level pump at all? I guess testing for 'are other runnables running' would do it...

The alternative fix is making sure that all places where we're pumping the queue we use filters checking for certain types of runnables.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Jonas, do you still have this fixed locally?  Or should I write up a simple patch to poll the event queue here?

Comment 20

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

i don't have a fix no, so feel free to take it

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 22

[Boris Zbarsky [:bzbarsky]]

It'd be great to have a testcase that can land as a regression test (all CSS, HTML, XBL, whatever is needed in one).

Comment 23

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

I don't really think this is the right fix. First of all once this condition occurs we'll keep reposting and reposting the runnable, consuming 100% CPU, until the XHR finishes loading.

Second, this means that if a sync XHR is started when there are ctors queued we'll execute them right then, which clearly is not what we want.

Third, its not always safe to run scripts when we're pumping the eventqueue like this, is it?

I really think the solution here is to filter events somehow.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> First of all once this condition occurs we'll keep reposting and reposting the
> runnable, consuming 100% CPU,

That's correct.  As I said, I view this is a very rare case that we don't care about working well, really.  It just needs to sort of work.

If you really feel strongly about this part, we could set a timer to repost the event instead of just reposting the event.

> Second, this means that if a sync XHR is started when there are ctors queued
> we'll execute them right then, which clearly is not what we want.

Why not?  If script is running, ctors can execute at any time (e.g. via the script executing document.body.offsetHeight).

> Third, its not always safe to run scripts when we're pumping the eventqueue
> like this, is it?

It's always safe to run script off an event.  It has to be.  The event might be the timer event for a setTimeout.  If someone is pumping the event queue at a time when it's not safe to run scripts, that someone needs to stop doing that.

> I really think the solution here is to filter events somehow.

We don't have provisions for that, and this is the wrong point in the cycle for another rewrite of our event model, imo.

Comment 25

[Boris Zbarsky [:bzbarsky]]

And seriously, since this is a non-blocker, I'd like to spend as little time as possible on it.  I think the patch is an improvement in that it gives correct (if not optimal performance-wise) behavior.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Missing file

Need this to compile and all

Comment 27

[Boris Zbarsky [:bzbarsky]]

Checked in.  We should get the test into crashtest once that exists.

And we should probably get a followup filed on handling this better post-1.9....