Last Comment Bug 535926 - "ASSERTION: This is unsafe" with <iframe>, XBL
: "ASSERTION: This is unsafe" with <iframe>, XBL
Status: RESOLVED FIXED
[sg:critical?]
: assertion, fixed1.9.0.20, testcase, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks: 343943 531176
  Show dependency treegraph
 
Reported: 2009-12-18 20:07 PST by Jesse Ruderman
Modified: 2010-10-11 11:09 PDT (History)
16 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+
.7+
.7-fixed
.11+
.11-fixed


Attachments
testcase (561 bytes, text/html)
2009-12-18 20:07 PST, Jesse Ruderman
no flags Details
stack trace (13.97 KB, text/plain)
2009-12-18 20:07 PST, Jesse Ruderman
no flags Details
patch (4.09 KB, patch)
2010-04-24 07:00 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
v2 (4.19 KB, patch)
2010-04-26 10:03 PDT, Olli Pettay [:smaug]
bzbarsky: review+
Details | Diff | Splinter Review
+space (4.19 KB, patch)
2010-04-26 10:18 PDT, Olli Pettay [:smaug]
jonas: superreview+
Details | Diff | Splinter Review
+some comments (4.46 KB, patch)
2010-04-27 02:46 PDT, Olli Pettay [:smaug]
christian: approval1.9.2.7+
christian: approval1.9.1.11+
dveditz: approval1.9.0.next+
Details | Diff | Splinter Review

Description Jesse Ruderman 2009-12-18 20:07:08 PST
Created attachment 418470 [details]
testcase

###!!! ASSERTION: This is unsafe!: 'Error', file /Users/jruderman/mozilla-central/content/events/src/nsEventDispatcher.cpp, line 485

This is the assertion just added in bug 531176.

I'm not sure why I need the setTimeout.
Comment 1 Jesse Ruderman 2009-12-18 20:07:30 PST
Created attachment 418471 [details]
stack trace
Comment 2 Olli Pettay [:smaug] 2009-12-28 07:58:43 PST
So nsBrowserStatusFilter::OnStateChange calls some JS code which end up to
progressmeter.xml which dispatches ValueChange.

In this particular case I wonder why we even call OnStateChange.
I believe the document is already loaded but
#21 0x00007f82d1f3e2ec in nsDocument::BlockOnload (this=0x261b070) at /home/smaug/mozilla/mozilla_cvs/hg/src/content/base/src/nsDocument.cpp:7037
#22 0x00007f82d20e85c8 in nsBindingManager::AddToAttachedQueue (this=0x2158530, aBinding=0x226a5e0)
    at /home/smaug/mozilla/mozilla_cvs/hg/src/content/xbl/src/nsBindingManager.cpp:955
adds new request to load group which triggers the method call.

Investigating...
Comment 3 Olli Pettay [:smaug] 2009-12-28 08:06:18 PST
But still, I think we can't easily prevent progress listeners to dispatch
events.
Comment 4 Boris Zbarsky [:bz] 2009-12-28 10:34:27 PST
Hmm.  We should avoid calling progress listeners for BlockOnload calls if we can do it...

Maybe we should stop faking the loadgroup thing and just directly have BlockOnload/UnblockOnload poke the docloader or something?
Comment 5 Bob Clary [:bc:] 2009-12-30 04:44:32 PST
An example of this assertion can be seen on Windows 1.9.3 at least at http://www.slacker.com/
Comment 6 Olli Pettay [:smaug] 2010-01-15 01:24:41 PST
Not sure if this is sg:critical (at least when using the testcase).
That is happening in chrome, and during time we might get dom modifications anyway.
Comment 7 Olli Pettay [:smaug] 2010-01-15 01:25:23 PST
I wonder if, at least for now, the assertion should fire only if the
target of the event is in content.
Comment 8 Olli Pettay [:smaug] 2010-02-07 12:08:21 PST
Repeating this:
> Not sure if this is sg:critical (at least when using the testcase).
> That is happening in chrome, and during time we might get dom modifications
> anyway.

What in this bug makes this sg:critical ?
Comment 9 Jesse Ruderman 2010-02-08 00:20:27 PST
I only marked it as sg:critical because the assertion was added in an sg:critical bug.  If this bug isn't a security bug, let's unmark the whiteboard and make it public.
Comment 10 Olli Pettay [:smaug] 2010-02-09 04:10:34 PST
Well, I wouldn't remove the security flag from this bug, but
this just may not be sg:critical.
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2010-04-13 15:43:58 PDT
Olli, if this only results in events being fired at chrome windows, then why not remove the security flag?
Comment 12 Johnny Stenback (:jst, jst@mozilla.com) 2010-04-14 16:20:31 PDT
Olli, any more thoughts on whether this needs to be sg:critical, or if it's even a security bug any more?
Comment 13 Olli Pettay [:smaug] 2010-04-15 01:03:29 PDT
comment 4 says what should be done here, I think.

I think this doesn't need to be sg:critical, since there is no clear evidence
that content can trigger anything bad - unless ff ui or some extension starts
to handle status updates in a different way.
But this is still a security bug.
Comment 14 Damon Sicore (:damons) 2010-04-20 13:39:26 PDT
Looks like we should probably remove the sg:crit rating here, but there's ongoing discussion of what the ration should be.  Thoughts?
Comment 15 Brandon Sterne (:bsterne) 2010-04-20 13:40:33 PDT
If this is a critical vulnerability only exposed to extension code presently, we should lower the severity to sg:high.  If we think it's likely that FF UI code would add changes triggering this vuln, then sg:critical severity is appropriate.
Comment 16 Olli Pettay [:smaug] 2010-04-20 13:44:16 PDT
Or we can just fix this and not care too much about sg:*. :)

Taking. I'll try to write the patch on Thursday.
Comment 17 Olli Pettay [:smaug] 2010-04-22 06:53:46 PDT
Um, I can't reproduce this atm.
I've tried on Linux and OSX, with and without html5 parser.

The assertion in this case was changed to a warning, but I can't see that either.

I wonder if bug 557398 changed something here.
Testing...
Comment 18 Olli Pettay [:smaug] 2010-04-22 06:59:30 PDT
Backing out bug 557398 doesn't seem to affect to this bug.
Comment 19 Olli Pettay [:smaug] 2010-04-22 07:02:03 PDT
Ah, perhaps LazyFrameCtor changed timing here. Which means this would still
need to be fixed in 1.9.2.
Comment 20 Olli Pettay [:smaug] 2010-04-22 11:11:31 PDT
Not LazyFrameCtor. Still trying to find a build where I get the error
Comment 21 Olli Pettay [:smaug] 2010-04-22 13:32:21 PDT
I can reproduce this at least on 1.9.2, when I added the same assertion/warning
what trunk has.
But patch needs to wait at least til tomorrow.
Comment 22 Olli Pettay [:smaug] 2010-04-24 07:00:35 PDT
Created attachment 441281 [details] [diff] [review]
patch

I posted this to tryserver
Comment 23 Olli Pettay [:smaug] 2010-04-25 07:41:59 PDT
Comment on attachment 441281 [details] [diff] [review]
patch

Passed tests on tryserver.

Boris, what do you think about this approach?
A bit ugly, but since I think we want this to 1.9.2 too, I didn't
want to make huge changes. And I'm not even sure what would be
the best way to change nsDocLoader or something.
It would be quite hackish to add onloadblocker-type-of-requests
and handle them specially in docloader.
 
This relies on script blockers to be on stack and 
PostUnblockOnloadEvent to handle things truly async.
Comment 24 Boris Zbarsky [:bz] 2010-04-26 09:32:35 PDT
Would it make sense to do:

  if (!nsContentUtils::IsSafeToRunScript()) {
    ++mAsyncOnloadBlockCount;
    if (mAsyncOnloadBlockCount == 1) {
      nsContentUtils::AddScriptRunner(
         NS_NEW_RUNNABLE_METHOD(nsDocument, this, AsyncBlockOnload));
    }
    return;
  }

and have AsyncBlockOnload call BlockOnload mAsyncOnLoadBlockCount times, so as not to thrash things with lots of scriptrunners?

Having an API on docloader (whose job it is to keep track of when onload should fire) to block onload would be a lot less hacky than having this fake request, btw.  But making it work right might be a pain...

Why is the new block in DoUnblockOnload needed?  How can we get there with mAsyncOnloadBlockCount != 0?
Comment 25 Olli Pettay [:smaug] 2010-04-26 09:41:15 PDT
(In reply to comment #24)

> Would it make sense to do:
> ...
> and have AsyncBlockOnload call BlockOnload mAsyncOnLoadBlockCount times, so as
> not to thrash things with lots of scriptrunners?
That should work.


> Why is the new block in DoUnblockOnload needed?  How can we get there with
> mAsyncOnloadBlockCount != 0?
If some script runner (which is added before the AsyncBlockOnload script runner, so it runs before it) processes event loop.
Comment 26 Boris Zbarsky [:bz] 2010-04-26 09:46:57 PDT
Ah, ok.  Do you want to post a patch with my suggestion from the beginning of comment 24?
Comment 27 Olli Pettay [:smaug] 2010-04-26 09:53:27 PDT
Yeah, I will, in a minute.
Comment 28 Olli Pettay [:smaug] 2010-04-26 10:03:18 PDT
Created attachment 441519 [details] [diff] [review]
v2

Like this?
Comment 29 Boris Zbarsky [:bz] 2010-04-26 10:15:30 PDT
Comment on attachment 441519 [details] [diff] [review]
v2

Space after "while", please.
Comment 30 Olli Pettay [:smaug] 2010-04-26 10:18:14 PDT
Created attachment 441526 [details] [diff] [review]
+space

uh, how did I miss the space :/

Anyway, this is a security bug, so sr is needed.
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2010-04-26 15:08:46 PDT
Comment on attachment 441526 [details] [diff] [review]
+space

Please add documentation on the member describing how it works. Took reading this a few times to figure it out.

And add a comment to ::BlockOnload describing *why* we're entering the if-statement.
Comment 32 Olli Pettay [:smaug] 2010-04-27 02:46:54 PDT
Created attachment 441743 [details] [diff] [review]
+some comments
Comment 34 Reed Loden [:reed] (use needinfo?) 2010-05-05 15:05:40 PDT
Just wanting to get this on the wanted/blocking list for future 1.9.2 release, as per earlier comments stating that this also affects 1.9.2.
Comment 35 christian 2010-05-05 15:30:58 PDT
Does this affect 1.9.1 as well?
Comment 36 Timothy Nikkel (:tnikkel) 2010-06-12 00:08:13 PDT
Bug 569674 is another regression from this, we need to figure that out before landing this on branches. Sorry I didn't mark it sooner.
Comment 37 Timothy Nikkel (:tnikkel) 2010-06-12 06:13:16 PDT
Disregard comment 36, I was thinking this was a different bug.
Comment 38 christian 2010-06-24 13:39:22 PDT
Comment on attachment 441743 [details] [diff] [review]
+some comments

a=LegNeato for 1.9.2.6 and 1.9.1.11. Please land this on mozilla-1.9.2 default and mozilla-1.9.1 default.

Code freeze is this Friday @ 11:59 pm PST.
Comment 41 Al Billings [:abillings] 2010-07-15 16:58:19 PDT
Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7pre) Gecko/20100630 Namoroka/3.6.7pre ( .NET CLR 3.5.30729) using the attached testcase.
Comment 42 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-18 22:02:05 PDT
Comment on attachment 441743 [details] [diff] [review]
+some comments

Requesting approval1.9.0.next on this patch so that we can take it in upcoming Camino 2.0.x security and stability updates.  If approved, I'll handle the checkins, unless the patch author requests otherwise.
Comment 43 Daniel Veditz [:dveditz] 2010-07-22 19:20:30 PDT
Comment on attachment 441743 [details] [diff] [review]
+some comments

Approved for 1.9.0.20, a=dveditz
Comment 44 Smokey Ardisson (offline for a while; not following bugs - do not email) 2010-07-24 15:30:31 PDT
Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.837; previous revision: 3.836
done
Checking in content/base/src/nsDocument.h;
/cvsroot/mozilla/content/base/src/nsDocument.h,v  <--  nsDocument.h
new revision: 3.381; previous revision: 3.380
done
Comment 45 Jesse Ruderman 2010-10-11 11:09:23 PDT
Crashtest: http://hg.mozilla.org/mozilla-central/rev/524a5670133b

Note You need to log in before you can comment on or make changes to this bug.