Closed Bug 491063 Opened 11 years ago Closed 11 years ago

Mixed mode warning dialog (and other dialog windows) is sometimes not rendered

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mcsmurf, Assigned: smaug)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: fixed1.9.1, regression)

Attachments

(3 files, 5 obsolete files)

To reproduce:
0. Fetch Firefox trunk or 1.9.1 build
1. Make sure security.warn_viewing_mixed is set to true
2. Go to https://wiki.mozilla.org/Platform/2009-04-21#Blocker_Report and click on one of those bugzilla links
3. As soon as the bugzilla query has loaded, click the "Back" button in the browser and observe an empty warning dialog
(4. Click on "Forward" and "Back" again, now the warning should no longer be empty)

Note that Step 4 does not always seem to work, it works in a Firefox trunk build, but with a SeaMonkey 1.9.1 build, it's still broken at this point.
Attached image Screenshot
Flags: blocking1.9.1?
With a SeaMonkey 1.9.1 based build I get this assertion a few times when loading the Wiki page (with the working security dialog):
###!!! ASSERTION: unexpected parallel nsIWebProgress OnStateChange and/or OnLocationChange notification: 'mOnStateLocationChangeReentranceDetection == 1', file f:/mozilla/tree-hg/src/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp, line 613
and when pressing the Back button to go back to the Wiki page (with the broken security dialog):
###!!! ASSERTION: This is unsafe: 'nsContentUtils::IsSafeToRunScript()', file  f:/mozilla/tree-hg/src/mozilla/layout/base/nsDocumentViewer.cpp, line 1081
related? Bug 474673
Duplicate of this bug: 474673
Moving to Layout because of the dependency and as this also occasionally happens with other dialog windows (for example with the "Warning: Unresponsive Script" dialog).
Assignee: kaie → nobody
Component: Security: PSM → Layout
QA Contact: psm → layout
Summary: Mixed mode warning is sometimes empty → Mixed mode warning dialog (and other dialog windows) is sometimes not rendered
Did this also regress on 1.9.0.*?  Is it SeaMonkey only or does it also affect Firefox?
Affects SeaMonkey and Firefox.
The 1.9.0.* branch is a bit difficult to test (need to find some test case first that brings up a warning). A 1.9.0.* build does not seem to care that the wiki page includes images from http://, it just ignores that (no warning).
Smaug: you wrote the patch in bug 466057 that apparently caused this regression; can you look into it?
Assignee: nobody → Olli.Pettay
Flags: blocking1.9.1? → blocking1.9.1+
To be clear - the broken behaviour of a non-default piece of UI is a bug to fix, but doesn't block the release on its own.  This is marked blocking so that we can ensure that this isn't the symptom of some larger bustage in mixed content detection or anything.
Johnathan, this is happening with multiple (possibly all modal, with varying difficulty of reproduction) dialogs.  See comment 7.  It's clearly a symptom of core dom or layout bustage _somewhere_.
I doubt all. I'll test this, but my guess is that this happens if we
open dialogs at "wrong" times.
Blocks: 401155
I get also an assertion added in bug 401155, the one in DocumentViewerImpl::PermitUnload
Blocks: 467422
Probably similar to bug 482286 which shows a similar behavior for the cookies dialog?
Also the dialog for untrusted ssl certificates was shown blank for me a month ago while doing the FFT's.
(In reply to comment #4)
> ###!!! ASSERTION: unexpected parallel nsIWebProgress OnStateChange and/or
> OnLocationChange notification: 'mOnStateLocationChangeReentranceDetection ==
> 1', file
> f:/mozilla/tree-hg/src/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,
> line 613
Kaie, any comments to this assertion?
(In reply to comment #17)
> (In reply to comment #4)
> > ###!!! ASSERTION: unexpected parallel nsIWebProgress OnStateChange and/or
> > OnLocationChange notification: 'mOnStateLocationChangeReentranceDetection ==
> > 1', file
> > f:/mozilla/tree-hg/src/mozilla/security/manager/boot/src/nsSecureBrowserUIImpl.cpp,
> > line 613
I think this might be a regression from something else than Bug 466057.
Would be great if someone could try to find that out, though it requires
debug builds.
OS: Windows XP → All
Hardware: x86 → All
Jonas, Boris, starting img/object/etc. synchronously in BindToTree causes
problems. I see two ways to fix this. One is to allow chrome flushes even
when there are scriptblockers (I don't like this approach) and the other
is to start loading using a script runner.
I just posted a patch to tryserver to see if that affects perf numbers,
though after that kind of change we could actually fix bug 467422 and
speed up generic Bind/Unbind.

I still have no idea what has caused nsSecureBrowserUIImpl.cpp assertions.
Maybe they've been there always.
Attached patch scriptrunners in bind (obsolete) — Splinter Review
So something like this.
Jonas, Boris, any comments?
Attachment #376023 - Flags: review?(bzbarsky)
I think using a script runner here is an excellent idea, generally speaking.  I assume that BindToTree already asserts that a script blocker is active on entry?

That said, for images in particular this can lead to issues, because of the sync loading stuff image loader can do.  In particular, there are cases where we'd lose big on perf if the load happens after frame construction, which will always be the case with the script runner, right?  I've been meaning to make that better (by assuming an image frame if the state is "still need to load" or something)...  maybe we should just do that?
I did post the patch to try server and at least that didn't show any perf problems.
Do you happen to know any perf testcases for img loading?
(In reply to comment #22)
> I did post the patch to try server and at least that didn't show any perf
> problems.
> Do you happen to know any perf testcases for img loading?

I'd expect anything that did real damage to image loading would impact Tp3 pretty heavily since those pages are full of them, no?
The performance issue I'm talking about happens when a page preloads an image and then has a script suddenly create lots of copies of it (e.g. using 1x1 pixel images to draw things on screen).  With the new code this operation would be O(N^2) in number of images as things stand.  None of our tinderbox tests exercise this (assuming they exercise image preloading at all, which I rather doubt).
So basically it would make bug 455690 appear in a lot more cases.
This doesn't regress performance in the testcase. But I need to test this some more. Posted the patch to tryserver.
(In reply to comment #26)
> A testcase
> http://mozilla.pettay.fi/moztests/1x1image.html

JFI: At least for me this test brought up an empty modal sheet which i wasn't able to close. The browser locked up.
Attachment #376023 - Flags: review?(bzbarsky)
Attached patch with some IsInDoc checks (obsolete) — Splinter Review
Attachment #376023 - Attachment is obsolete: true
Attachment #376951 - Attachment is obsolete: true
Attachment #376996 - Flags: review?(bzbarsky)
Attachment #376996 - Flags: review?(bzbarsky)
Comment on attachment 376996 [details] [diff] [review]
with some IsInDoc checks

Actually, we don't want IsInDoc checks to keep the old behavior
Attachment #376951 - Attachment is obsolete: false
Attachment #376951 - Flags: review?(bzbarsky)
That doesn't seem to address my concern from comment 21, does it?
It does try to fix that.
comment 21 is basically http://mozilla.pettay.fi/moztests/1x1image.html, right?
And the patch clears broken state so that frame is created by default.
The first patch regress performance pretty badly with 1x1image.html.

Or is there some other case I'm not aware of?
Oh, I see.  It clears broken but doesn't set loading, I guess.  So then we'll set loading when the runnable runs, and reframe at that point.  Which means that it slightly slows down the case when the image is _not_ preloaded (the normal pageload case).  I assume you did run that last patch through Tp?
(In reply to comment #33)
> I assume you did run that last patch through Tp?
Yes, I did post it to tryserver. Didn't see any difference
OK.  Just looking at the images for now...  If the "src" attr is present but we can't create a URI object from it, this new code won't put the image in a broken state, will it?  That should be reftestable.  Try <img src="http://a b"> (with the space).

Same thing for cases when mLoadingEnabled is false in nsImageLoadingContent.  That ought to be mochitestable, at least.
(In reply to comment #35)
> OK.  Just looking at the images for now...  If the "src" attr is present but we
> can't create a URI object from it, this new code won't put the image in a
> broken state, will it?
Ah, right. nsImageLoadingContent::LoadImage doesn't handle that case, though
seems like one could trigger it even now if one sets .src first to some valid
value and then to some invalid one.

Ok, I need to check mLoadingEnabled somewhere.
(nsImageLoadingContent isn't very nice code :/ )
> if one sets .src first to some valid value and then to some invalid one.

In that case we continue showing the old image, more or less on purpose.

> (nsImageLoadingContent isn't very nice code :/ )

Patches accepted!  Its main problem is having to deal with the "sometimes I'm sync, sometimes not" behavior of imagelib, though.
Attached patch better (obsolete) — Splinter Review
Still no mochitests or reftest. Will do those tomorrow.
Attachment #376951 - Attachment is obsolete: true
Attachment #376996 - Attachment is obsolete: true
Attachment #376951 - Flags: review?(bzbarsky)
Attached patch With reftest (obsolete) — Splinter Review
Attachment #377263 - Attachment is obsolete: true
Attachment #378035 - Flags: review?(bzbarsky)
Attachment #378035 - Flags: review?(bzbarsky) → review+
Comment on attachment 378035 [details] [diff] [review]
With reftest

So overall, this looks really nice.  Some nits:

1)  NS_NEW_RUNNABLE_METHOD might be nice to use throughout (no need to repeat
    the class name then).  Either way is fine, though.
2)  This patch introduces a behavior change for images in the loading-disabled
    case: we used to fire an error event and now no longer do.  It might be
    better to post the runnable for images unconditionally and change the code
    it runs to something like:

    if (GetAttr(....) &&
        (NS_FAILED(LoadImage(....)) ||
	 !LoadingEnabled())) {
    }

    with a comment about making sure that LoadImage() happens before
    LoadingEnabled() is checked there.  Similar for image inputs, SVG images,
    etc.
3)  For HTML input, we should be checking that mType is NS_FORM_INPUT_IMAGE in
    MaybeLoadImage, just in case someone changes the type between BindToTree
    and this code running.
4)  We should file a followup bug (dependent on the HTML5 parser landing) to
    investigate removing the various image/object aNotify arguments, perhaps.
5)  We still have places where we start network loads while script blockers are
    active.  Loading of images from nsCSSDataBlock code comes to mind, and I
    seem to recall that user font loads behave the same way.  We should get
    followup bugs filed on these.

r+sr=bzbarsky with the nits picked.  Thanks for adding that reftest!
Except with that indent fixed, of course.  Darn tabs.
Moving to content-land since that's where the patch is.
Component: Layout → DOM
QA Contact: layout → general
Attached patch +nitsSplinter Review
Attachment #378035 - Attachment is obsolete: true
Attached patch 191Splinter Review
This is fixed, but I need to file the followups and
a bug about nsSecureBrowserUIImpl.cpp.

http://hg.mozilla.org/mozilla-central/rev/dbf96b1b73bd
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/40fab443e59b
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Whiteboard: [needs landing]
I can still get this to repro in 3.5pre on https://intranet.mozilla.org/forum/
About 1/3 of the time when I click through to a thread (many of which have HTTP avatars), I get a completely blank dialog that locks up the browser, requiring restart. 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Removing fixed1.9.1 for now. How confident are we that the 1.9.1 patch fixed the original problem? The reftest presumably failed without the patch, so I guess it does fix *a* problem, just not sure if we're confident that it fixed the problem Frank and Lucas are seeing.
Keywords: fixed1.9.1
https://intranet.mozilla.org/forum wasn't the testcase for this bug.
Please open a new one
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
I can reproduce the forum problem if I set 
security.warn_viewing_mixed to true and security.warn_viewing_mixed.show_once to false. Investigating.
Depends on: 494590
Bug 494590 filed on the forums issue.
No longer depends on: 502168
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.