Closed
Bug 1230508
Opened 9 years ago
Closed 9 years ago
CRASH: Firefox crashes with Pixel Perfect extension
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: Honza, Assigned: xidorn)
References
Details
(Keywords: regression)
Attachments
(1 file)
|
(deleted),
text/x-review-board-request
|
dbaron
:
review+
ritu
:
approval-mozilla-beta+
|
Details |
STR:
1) Download Pixel Perfect from https://addons.mozilla.org/cs/firefox/addon/pixel-perfect/
2) Install into DevEdition (it crashes in Nightly too), make sure e10s is on
3) Click on the Pixel Perfect button in Firefox Toolbar
4) Click on Add Layer button in the Pixel Perfect popup window (pick an image)
5) Click the Pixel Perfect toolbar button again to hide the popup -> CRASH
Here is my crash report:
https://crash-stats.mozilla.com/report/index/f7df94e0-72fe-4584-ab6e-1dcbf2151204
It might be related to anonymous content.
Honza
|
||
Comment 1•9 years ago
|
||
(not sure why this was filed as a DOM bug)
Component: DOM: Core & HTML → Layout
|
Reporter | |
Comment 2•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #1)
> (not sure why this was filed as a DOM bug)
Sorry, I thought it's related to the anonymous content.
Thanks, for fixing that.
Honza
|
|
||
Comment 3•9 years ago
|
||
Btw, is this a regression?
|
||
Comment 4•9 years ago
|
||
> 3) Click on the Pixel Perfect button in Firefox Toolbar
When I do that in a nightly I get two assertions:
[Parent 49826] ###!!! ASSERTION: Deprecated, use NewChannel2 providing loadInfo arguments!: 'false', file /Users/bzbarsky/mozilla/inbound/mozilla/netwerk/base/nsIOService.cpp, line 932
[Parent 49826] ###!!! ASSERTION: Please pass security info when creating a channel: 'loadInfo', file /Users/bzbarsky/mozilla/inbound/mozilla/netwerk/base/nsIOService.cpp, line 858
and nothing shows in the popup window.
|
|
||
Comment 5•9 years ago
|
||
Ah, if I wait 10s or so something shows up in that window...
So if I follow the steps to reproduce, I get a crash in the content process, not the chrome one. First, two nonfatal assertions:
[Child 49852] ###!!! ASSERTION: not in child list: 'found', file /Users/bzbarsky/mozilla/inbound/mozilla/layout/base/nsLayoutUtils.cpp, line 1276
[Child 49852] ###!!! ASSERTION: unexpected child list: 'aListID == kPrincipalList', file /Users/bzbarsky/mozilla/inbound/mozilla/layout/generic/nsViewportFrame.cpp, line 189
then:
Assertion failure: aListID == kPrincipalList || aListID == kNoReflowPrincipalList (unexpected child list), at /Users/bzbarsky/mozilla/inbound/mozilla/layout/generic/nsContainerFrame.cpp:125
What we have is aListID=mozilla::layout::kAbsoluteList.
So someone is trying to remove an absolute child from the viewport, which the viewport does not expect.
Thing is, it doesn't expect that because there should be no absolute kids of the viewport, only fixed kids.
And in fact, the frame being removed _is_ on the fixed list of the viewport. But its style definitely has "position: absolute".
The placeholder for this abs pos thing is inside a block whose mContent is <html>. The parent of this block is the nsCanvasFrame. The primary frame of <html> is this block. So the placeholder bits are all reasonable. So the only question is why this abs pos frame is ending up on the viewport's fixed list to start with...
|
|
||
Comment 6•9 years ago
|
||
Alright. So looks like we land in nsFrameConstructorState::ProcessFrameInsertions with a call like so:
#0 nsFrameConstructorState::ProcessFrameInsertions (this=0x7fff5fbf9ba8, aFrameItems=@0x7fff5fbf9c20, aChildListID=mozilla::layout::kFixedList) at nsCSSFrameConstructor.cpp:1267
but we have:
(gdb) p aFrameItems.mFirstChild->StyleDisplay()->mPosition
$11 = 2 '\002'
That's abs pos, not fixed pos. After this, things will go sour.
The caller is here:
997 ProcessFrameInsertions(mTopLayerItems, nsIFrame::kFixedList);
and the comment before that says:
996 // Items in the top layer are fixed positioned children of the viewport frame.
We should either enforce that somehow or not create frames for non-fixed-pos top layer items, or something.
Flags: needinfo?(quanxunzhen)
|
|
Reporter | |
Comment 7•9 years ago
|
||
Btw. I have been testing with Pixel Perfect 2.0.8
https://github.com/firebug/pixel-perfect/releases/tag/pixelperfect-2.0.8
Not sure, if that's important, but just to make sure we have the same config.
Honza
|
|
||
Comment 8•9 years ago
|
||
Looks like in nsRuleNode::ComputeDisplayData we have:
5723 // If an element is put in the top layer, while it is not absolutely
5724 // positioned, the position value should be computed to 'absolute' per
5725 // the Fullscreen API spec.
5726 if (display->mTopLayer != NS_STYLE_TOP_LAYER_NONE &&
5727 !display->IsAbsolutelyPositionedStyle()) {
5728 display->mPosition = NS_STYLE_POSITION_ABSOLUTE;
which is what I assume is happening in this case.
The frame coming through as described in comment 6 seems to have the following rules applied to it:
div:-moz-native-anonymous.moz-custom-content-container {
position: absolute;
}
(from ua.css in the extension)
div:-moz-native-anonymous.moz-custom-content-container {
pointer-events: none;
-moz-top-layer: top;
position: fixed;
top: 0;
left: 0;
width: 100%;
height: 100%;
}
(from our built-in ua.css)
So the extension is messing with UA styles it should really not mess with. I guess we could just make that UA style !important, but of course the extension could still override it if it tried hard enough.
|
|
||
Comment 9•9 years ago
|
||
Oh, and so I was wrong about the nsRuleNode::ComputeDisplayData bit. But that bit still seems broken to me, since we place the top-layer items in the fixed list, no?
|
Assignee | |
Comment 10•9 years ago
|
||
From the perspective of the spec, an top layer element could be either fixed or absolute element. For fixed top layer element, its containing block is the viewport frame, and for absolute ones, that is the initial containing block.
For fullscreen, only the fixed half is needed, and thus only that part is implemented currently, but the rule computation code still allows absolute per spec, and the behavior there can be unexpected.
We will end up implementing absolute part as well when we implement <dialog>. For this bug, I guess adding !important to the ua style should be enough.
What I'm concerned is that, :-moz-native-anonymous and -moz-top-layer are both UA stylesheet only. If addon can mess up styles of :-moz-native-anonymous, it can also do so with -moz-top-layer. So I guess I probably should fix the computation code to always force "position: fixed" for top layer element for now.
Flags: needinfo?(quanxunzhen)
|
|
Assignee | |
Comment 11•9 years ago
|
||
anyway, leave a ni? for myself to fix this issue.
Flags: needinfo?(quanxunzhen)
|
|
Assignee | |
Comment 12•9 years ago
|
||
Bug 1230508 - Always compute position to fixed for top layer elements for now.
Although the spec says absolute is allowed for top layer elements, and actually
other values should be computed to absolute, but this is mostly fine because
the only way we currently support for web content to use the top layer is via
the Fullscreen API, however, fullscreen elements are forced to be fixed by the
UA sheet.
Given only fixed is safe for top layer element currently, rather than doing
what the spec says, we should prefer always force it, until we really add
support for the other value.
Attachment #8696543 -
Flags: review?(dbaron)
|
|
Assignee | |
Comment 13•9 years ago
|
||
This may make the behavior of the addon incorrect, but that's better than crashing the browser anyway.
Flags: needinfo?(quanxunzhen)
|
||
Comment 14•9 years ago
|
||
Comment on attachment 8696543 [details]
MozReview Request: Bug 1230508 - Always compute position to fixed for top layer elements for now.
https://reviewboard.mozilla.org/r/27365/#review24747
::: layout/style/nsRuleNode.cpp:5727
(Diff revision 1)
> - !display->IsAbsolutelyPositionedStyle()) {
> - display->mPosition = NS_STYLE_POSITION_ABSOLUTE;
> + // XXX We currently only support fixed top layer element. But per
> + // spec it should check IsAbsolutelyPositionedStyle() instead of
Could you file a bug on fixing this (file that if needed), and change this "XXX" to "FIXME (bug NNNNNNN)"
Also, is there a bug on exposing top-layer to the Web? Does such a thing even make sense? If there is, the new bug should block that.
Attachment #8696543 -
Flags: review?(dbaron) → review+
|
|
Assignee | |
Comment 15•9 years ago
|
||
(In reply to David Baron [:dbaron] ⌚UTC-5 from comment #14)
> Comment on attachment 8696543 [details]
> MozReview Request: Bug 1230508 - Always compute position to fixed for top
> layer elements for now.
>
> https://reviewboard.mozilla.org/r/27365/#review24747
>
> ::: layout/style/nsRuleNode.cpp:5727
> (Diff revision 1)
> > - !display->IsAbsolutelyPositionedStyle()) {
> > - display->mPosition = NS_STYLE_POSITION_ABSOLUTE;
> > + // XXX We currently only support fixed top layer element. But per
> > + // spec it should check IsAbsolutelyPositionedStyle() instead of
>
> Could you file a bug on fixing this (file that if needed), and change this
> "XXX" to "FIXME (bug NNNNNNN)"
Well, we'll need to support this when we implement modal <dialog>, and we do not need to support this until then, so I don't really think we need a separate bug for this. Probably just refer to this bug or bug 840640.
> Also, is there a bug on exposing top-layer to the Web? Does such a thing
> even make sense? If there is, the new bug should block that.
I consider the top layer is designed to allow using DOM to put something on the topmost regardless the existing stacking. I don't think it really makes sense to expose this concept to the web directly, because the web content can already use z-index to do this.
|
|
Assignee | |
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8707e6e5ac80be8de3f012564a2d418d8d93c1d
Bug 1230508 - Always compute position to fixed for top layer elements for now. r=dbaron
|
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → quanxunzhen
|
|
Assignee | |
Comment 17•9 years ago
|
||
This addon uses absolute, which means this patch would likely break the addon's behavior, which is better than crash, I suppose.
|
||
Comment 18•9 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
|
|
Assignee | |
Comment 19•9 years ago
|
||
This is actually a regression from bug 1215360, because before that bug, we only use top layer on fullscreen, which has "position: fixed !important".
|
|
Assignee | |
Comment 20•9 years ago
|
||
Comment on attachment 8696543 [details]
MozReview Request: Bug 1230508 - Always compute position to fixed for top layer elements for now.
Approval Request Comment
[Feature/regressing bug #]: bug 1215360
[User impact if declined]: some addons which uses anonymous content with non-fixed position may crash the browser
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low risk, this change only disables something we don't currently support at all
[String/UUID change made/needed]: n/a
Attachment #8696543 -
Flags: approval-mozilla-beta?
Honza, could you please confirm that this crash is fixed on the latest Nightly build? This will give me some confidence uplifting to Beta44. Thanks!
Flags: needinfo?(odvarko)
Comment on attachment 8696543 [details]
MozReview Request: Bug 1230508 - Always compute position to fixed for top layer elements for now.
Fixes a crash, has been in Nightly for a few days, safe to uplift to Beta44.
Attachment #8696543 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
|
||
Comment 23•9 years ago
|
||
| bugherder uplift | ||
|
||
Comment 24•9 years ago
|
||
| bugherder uplift | ||
status-b2g-v2.5:
--- → fixed
|
|
Reporter | |
Comment 25•9 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #21)
> Honza, could you please confirm that this crash is fixed on the latest
> Nightly build? This will give me some confidence uplifting to Beta44. Thanks!
I can confirm that the crash is fixed, thanks!
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> This addon uses absolute, which means this patch would likely break the
> addon's behavior, which is better than crash, I suppose.
Yes, the addon is now broken.
Is there any way to keep the original behavior?
All the add-on needs is display an image (using anonymous content) that scrolls together with the content (i.e. do *not* use fixed position).
Honza
Flags: needinfo?(odvarko) → needinfo?(quanxunzhen)
|
|
Assignee | |
Comment 26•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #25)
> Is there any way to keep the original behavior?
> All the add-on needs is display an image (using anonymous content) that
> scrolls together with the content (i.e. do *not* use fixed position).
Unfortunately, currently no direct way via CSS. You may want to listen to scroll event and change the top / left values according to scrollTop / scrollLeft instead. Sorry about the additional complexity.
We are going to support 'position: absolute' on the top layer (due to <dialog>) at some point in the future, but it is nontrivial work and even if we do it now, it is not upliftable anyway.
That also indicates that you may want to change the 'position' value to 'fixed' in addition to reacting to scrolling, otherwise it could be broken again when we start supporting 'absolute' there.
Flags: needinfo?(quanxunzhen)
|
|
Reporter | |
Comment 27•9 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #26)
> (In reply to Jan Honza Odvarko [:Honza] from comment #25)
> > Is there any way to keep the original behavior?
> > All the add-on needs is display an image (using anonymous content) that
> > scrolls together with the content (i.e. do *not* use fixed position).
>
> Unfortunately, currently no direct way via CSS. You may want to listen to
> scroll event and change the top / left values according to scrollTop /
> scrollLeft instead. Sorry about the additional complexity.
OK, I see
>
> We are going to support 'position: absolute' on the top layer (due to
> <dialog>) at some point in the future, but it is nontrivial work and even if
> we do it now, it is not upliftable anyway.
Is there any existing bug report I could follow, so I am notified when
this happens?
>
> That also indicates that you may want to change the 'position' value to
> 'fixed' in addition to reacting to scrolling, otherwise it could be broken
> again when we start supporting 'absolute' there.
Yes
Honza
|
|
Assignee | |
Comment 28•9 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #27)
> > We are going to support 'position: absolute' on the top layer (due to
> > <dialog>) at some point in the future, but it is nontrivial work and even if
> > we do it now, it is not upliftable anyway.
> Is there any existing bug report I could follow, so I am notified when
> this happens?
Filed bug 1236828.
You need to log in
before you can comment on or make changes to this bug.
Description
•