In bug 1361235 comment 3, Emilio notices that StyleIterator and AllChildIterator don't find the anonymous content created by the PopupFrame of a <select> element. Should be iterating over all the child frames of the element to find non-primary frames that could also have created anonymous content? This might prevent us from restyling content in select popups properly.

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. https://reviewboard.mozilla.org/r/138998/#review142628 ::: dom/base/nsContentUtils.cpp (Diff revision 1) > - // Don't wait until UnbindFromTree to clear ServoElementData, since > - // leak checking at shutdown can run before the AnonymousContentDestroyer > - // runs. > - if ((*aContent)->IsStyledByServo() && (*aContent)->IsElement()) { > - ServoRestyleManager::ClearServoDataFromSubtree((*aContent)->AsElement()); > - } Nice! So the comment wasn't correct, and the leaks were actually because we were missing these in the iterator?

Doesn't that patch make the _normal_ case (some random element in the DOM) walk its entire frame childlist looking for the anon bits? If so, that seems pretty suboptimal to me.

Hm, I guess I was thinking about how we optimize out the AllChildIterator for most cases in stylo, but I guess it's true that there are C++ consumers that use it unconditionally. Any counter-proposals, or at least sketches of one?

At least an element bit (or a property) that we set when the non-primary frame generates anon content seems on point. Or, if there are a reduced set of frames that can create such children (PopupFrame, etc), checking the frame type should be ok.

Yeah, that's reasonable. I'll check the primary frame type and add assertions to catch any new AC-creating-non-primary-frames.

Still working on some failures due to the assertions I add in this patch, but this is the direction I'm currently trying.

Ah, looks like I've got just a couple more assertions to fix, from this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2ed93e1429cf5860612c6a08095c52efcc673fa&group_state=expanded

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. Tracked down all the assertions now.

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. https://reviewboard.mozilla.org/r/138998/#review143904 ::: dom/base/ChildIterator.h:235 (Diff revision 3) > > private: > // Helpers. > void AppendNativeAnonymousChildren(); > void AppendNativeAnonymousChildrenFromFrame(nsIFrame* aFrame); > + void AppendNativeAnonymousChildrenFromFramesForContent(nsIFrame* aFrame); Obviously this line should no longer be here.

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. I'll defer to bz on this one.

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. https://reviewboard.mozilla.org/r/138998/#review142628 > Nice! So the comment wasn't correct, and the leaks were actually because we were missing these in the iterator? Forgot to reply to this before, but yes, the comment was wrong.

This approach seems really fragile if we change the frame structure somewhere... That said, do we want to handle this via AllChildrenIterator, or via the anon-box machinery we already have? That machinery already allows us to reach the scrollframe in the fieldset case, various things under the scrollframe (e.g. the scrolledcontent, but we could also walk the scrollbars) in the scrollframe case, etc. And I thought it already walked the popup frame of the combobox to update its style. So is something else, not style updates, that uses AllChildrenIterator, failing? Or is the problem "just" that scrollframe should update the style of its scrollbars, not just its scrollcontent, if the scrollframe is not the primary frame of the element? I guess there's also the detailsframe part... By the way, here's a specific example that doesn't entirely make sense to me: the code in this patch walks to the first child of a columnset. But if you have a scrollable columnated fieldset, we won't reach the columnset at all via this code, as far as I can tell. So which of those cases is wrong?

Still thinking about this, but one thing I was considering that doesn't quite work: we could have non-primary frames stash their anonymous content on the element when they create it, in an element property. Probably with a bit for whether it's there; we could add one by fixing the dir stuff. But I don't think frames know whether they're the primary frame when they get CreateAnonymousContent called; that happens before the primary frame pointer is set on the element. Of course we could have all anon-content-creating frames unconditionally set this stuff on the element, just appending things to the list as needed, and clearing the list on frame destruction. It's a bit annoying that we would have to do that for the "normal scrollframe" case, though...

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #16) > This approach seems really fragile if we change the frame structure > somewhere... It is. :( It would be better if I could make the assertions always run, not just in AllChildrenIterator. It's probably not great to do it under SetPrimaryFrame... > That said, do we want to handle this via AllChildrenIterator, or via the > anon-box machinery we already have? That machinery already allows us to > reach the scrollframe in the fieldset case, various things under the > scrollframe (e.g. the scrolledcontent, but we could also walk the > scrollbars) in the scrollframe case, etc. And I thought it already walked > the popup frame of the combobox to update its style. So is something else, > not style updates, that uses AllChildrenIterator, failing? Or is the > problem "just" that scrollframe should update the style of its scrollbars, > not just its scrollcontent, if the scrollframe is not the primary frame of > the element? I guess there's also the detailsframe part... The AllChildrenIterator use that's not working here is the teardown stuff in ServoStyleSet::BeginShutdown, so clearing out existing ServoElementData in the document. I guess somehow factoring out and re-using the UpdateStyleOfChildAnonBox stuff feels like a similar level of fragility as the patch I've got. Well, it does have the advantage of the logic for finding the anon boxes in the frame classes themselves. > By the way, here's a specific example that doesn't entirely make sense to > me: the code in this patch walks to the first child of a columnset. But if > you have a scrollable columnated fieldset, we won't reach the columnset at > all via this code, as far as I can tell. So which of those cases is wrong? Yeah, we don't reach the columnset in that case. I guess we don't have any tests for scrollable columated fieldsets... (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #17) > But I don't think frames know whether they're the primary frame when they > get CreateAnonymousContent called; that happens before the primary frame > pointer is set on the element. Yes, initially I tried to set a bit when we create NAC for non-primary frames, but also ran into this problem of content not knowing what its primary frame is by that point.

(In reply to Cameron McCormack (:heycam) from comment #18) > Yeah, we don't reach the columnset in that case. I guess we don't have any > tests for scrollable columated fieldsets... Though maybe that doesn't matter, since the frame tree looks like: FieldSet (primary) HTMLScroll ColumnSet Block so there's no need to find the ColumnSet frame.

Though it's still a problem with a scrollable, columnated <details>. Another option would be to search for the same-content non-primary NAC-creating frames, like in the first patch, but only at SetPrimaryFrame time.

After sleeping on it, I think it does make sense to rework the anon box restyling machinery to help here.

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. https://reviewboard.mozilla.org/r/138998/#review152062 I'd like to at least see the patch with updated comments. ::: layout/generic/nsFrame.cpp:10502 (Diff revision 4) > void > nsIFrame::DoUpdateStyleOfOwnedAnonBoxes(ServoStyleSet& aStyleSet, > - nsStyleChangeList& aChangeList, > + nsStyleChangeList& aChangeList, > - nsChangeHint aHintForThisFrame) > + nsChangeHint aHintForThisFrame) > { > + AutoTArray<nsIFrame*,4> frames; OK, so as I see it we have three basic options here. We can take the approach in this patch, where we just have anything with anon boxes throw them all in the list and then this code does a switch on type to determine what to do with the anon boxes. This works for the moment, I guess, but we're going to add more things that need the special processing... that said, they won't be hard to distinguish based on their type, I think. And they can always be distinguished based on their pseudo-element style, of course. The second option is to make assumptions about which anon boxes do or do not create NAC (in particular, table outer frame and ib-split blocks do not create NAC). But that seems a bit fragile... The third option is do something like an array of (frame, function-pointer) with the latter allowed to be null to indicate "call UpdateStyleOfChildAnonBox". That would allow the subclass-specific logic and special-casing to be completely encapsulated in subclasses, at the cost of an extra indirect function call in the two cases that need it right now. I think I've convinced myself we can live with approach 1 for now, so I'll go ahead and review this patch as-is, but if you think one of the other two options is better I would not at all object to it! I think no matter which option we pick here it won't cover the "wrapper" anon boxes like anonymous tables and ruby bits, but I think those won't create NAC either. I hope. ::: layout/generic/nsFrame.cpp:10509 (Diff revision 4) > + for (nsIFrame* f : frames) { > + switch (f->Type()) { > + case LayoutFrameType::TableWrapper: > + // We handle the style resolution of this specially, since it needs > + // to inherit from its child, the nsTableFrame. > + static_cast<nsTableWrapperFrame*>(f)->InnerTableFrame()-> You could just cast `this` to `nsTableFrame*`, right? And assert that `Type()` is the right thing. ::: layout/generic/nsFrame.cpp:10518 (Diff revision 4) > + case LayoutFrameType::Block: > + if (f->GetStateBits() & NS_FRAME_PART_OF_IBSPLIT) { > + // This is the first anonymous block frame for an {ib} split. We > + // handle this specially, to avoid recomputing a new style context for > + // each of the continuations / IB split siblings. > + nsIFrame* inlineFrame = Again, this is a really complicated way of writing `this`. ::: layout/generic/nsFrame.cpp:10551 (Diff revision 4) > + size_t i = aResult.Length(); > + AppendDirectlyOwnedAnonBoxes(aResult); > + while (i < aResult.Length()) { > + nsIFrame* f = aResult[i]; > + if (f->GetStateBits() & NS_FRAME_OWNS_ANON_BOXES) { > + f->AppendDirectlyOwnedAnonBoxes(aResult); The fact that this is not calling f->DoAppendOwnedAnonBoxes() is correct, but very non-obvious. It works because we're appending to aResult so will end up walking the newly appended frames and asking for their AppendDirectlyOwnedAnonBoxes directly as needed, right? Please document this clearly. I'd been going to suggest changes to this function that would have broken it, until I figured out what's going on. Also please document clearly why this is not a ranged loop over aResult (same reason, of course). ::: layout/generic/nsIFrame.h:3266 (Diff revision 4) > DoUpdateStyleOfOwnedAnonBoxes(aStyleSet, aChangeList, aHintForThisFrame); > } > } > > +protected: > + void DoUpdateStyleOfOwnedAnonBoxes(mozilla::ServoStyleSet& aStyleSet, This needs documentation. ::: layout/generic/nsIFrame.h:3290 (Diff revision 4) > // Returns the generated change hint for the frame. > nsChangeHint UpdateStyleOfOwnedChildFrame(nsIFrame* aChildFrame, > nsStyleContext* aNewStyleContext, > nsStyleChangeList& aChangeList); > > + void AppendOwnedAnonBoxes(nsTArray<nsIFrame*>& aResult) { This needs documentation too. ::: layout/generic/nsIFrame.h:3296 (Diff revision 4) > + if (GetStateBits() & NS_FRAME_OWNS_ANON_BOXES) { > + DoAppendOwnedAnonBoxes(aResult); > + } > + } > + > + void DoAppendOwnedAnonBoxes(nsTArray<nsIFrame*>& aResult); And this. ::: layout/generic/nsIFrame.h:3299 (Diff revision 4) > * Hook subclasses can override to actually implement updating of style of > * owned anon boxes. And this comment is no longer quite correct, since we now rely on this hook for other things too. Please update the docs, and in particular how this differs from DoAppendOwnedAnonBoxes. ::: layout/generic/nsInlineFrame.cpp:1034 (Diff revision 4) > + "We should be the primary frame for our element"); > + > + nsIFrame* blockFrame = > + PresContext()->PropertyTable()->Get(this, nsIFrame::IBSplitSibling()); > + MOZ_ASSERT(blockFrame, "Why did we have an IB split?"); > + aResult.AppendElement(blockFrame); So here's the thing. This is either too much or not enough. If the ib-split blocks _can_ have NAC hanging off them somewhere, then why is appending only the first-in-flow OK here? If they can't, why do we need to append anything at all? We could just as easily switch on the frame type in the "update styles" code and if inline frame call into UpdateStyleOfOwnedAnonBoxesForIBSplit, and not do these extra hashtable lookups...

Comment on attachment 8869975 [details] Bug 1364361 - Part 2: Make AllChildIterator find NAC created by all of an element's anon boxes. https://reviewboard.mozilla.org/r/141524/#review152066 r=me

Comment on attachment 8869976 [details] Bug 1364361 - Part 3: Remove now-unnecessary clearing of ServoElementData from anonymous content destroyer callback. https://reviewboard.mozilla.org/r/141526/#review152068 r=me

Comment on attachment 8869977 [details] Bug 1364361 - Part 4: Add crashtest for a scrollable, multicol <details> element. https://reviewboard.mozilla.org/r/141528/#review152070 r=me with the nits below. ::: layout/generic/crashtests/1364361-1.html:4 (Diff revision 1) > +<!DOCTYPE html> > +<style> > +details { > + columns-count: 3; column-count, not columns-count, right? Did this crashtest fail without the other patches? ::: layout/generic/crashtests/1364361-1.html:8 (Diff revision 1) > +details { > + columns-count: 3; > + column-width: 5em; > + background-color: yellow; > + overflow: scroll; > + /* overflow: -moz-hidden-unscrollable; */ Why is this commented-out line here?

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #27) > The third option is do something like an array of (frame, function-pointer) > with the latter allowed to be null to indicate "call > UpdateStyleOfChildAnonBox". That would allow the subclass-specific logic > and special-casing to be completely encapsulated in subclasses, at the cost > of an extra indirect function call in the two cases that need it right now. > > I think I've convinced myself we can live with approach 1 for now, so I'll > go ahead and review this patch as-is, but if you think one of the other two > options is better I would not at all object to it! Having the special casing in the subclasses sounds good to me. I'll try option 3.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #27) > ::: layout/generic/nsInlineFrame.cpp:1034 > (Diff revision 4) > > + "We should be the primary frame for our element"); > > + > > + nsIFrame* blockFrame = > > + PresContext()->PropertyTable()->Get(this, nsIFrame::IBSplitSibling()); > > + MOZ_ASSERT(blockFrame, "Why did we have an IB split?"); > > + aResult.AppendElement(blockFrame); > > So here's the thing. This is either too much or not enough. If the > ib-split blocks _can_ have NAC hanging off them somewhere, then why is > appending only the first-in-flow OK here? If they can't, why do we need to > append anything at all? We could just as easily switch on the frame type in > the "update styles" code and if inline frame call into > UpdateStyleOfOwnedAnonBoxesForIBSplit, and not do these extra hashtable > lookups... If you're happy to have this one special case check up in nsIFrame::DoUpdateStyleOfOwnedAnonBoxes, then sure.

I think I am, if we document why it's done that way.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #30) > Did this crashtest fail without the other patches? Just checked (again), and yes, it causes a leak. https://treeherder.mozilla.org/#/jobs?repo=try&revision=499804b512730a002917afa949252b2085e892bf

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1135319f7655a26f46f351928c23296585980c2 (forgot to update part 2)

Comment on attachment 8869975 [details] Bug 1364361 - Part 2: Make AllChildIterator find NAC created by all of an element's anon boxes. https://reviewboard.mozilla.org/r/141524/#review152278 We're running static analysis on all MozReview requests as an experiment, and this particular request triggered an error that has us puzzled: Error: no viable conversion from 'AutoTArray<nsIFrame *, 8>' to 'nsTArray<nsIFrame::OwnedAnonBox>' [clang-diagnostic-error] at dom/base/ChildIterator.cpp:379:40 primaryFrame->AppendOwnedAnonBoxes(ownedAnonBoxes); ^ Here are the complete error details: https://irccloud.mozilla.com/pastebin/Pz5nQfOa/nstarray-error.txt "clang-diagnostic-error" makes it look like a configuration error with our bot (e.g. missing library), however all the referenced code location are within our tree/obj-dir. Do you have any idea why clang-tidy would fail to understand the type of `ownedAnonBoxes`?

Comment on attachment 8869975 [details] Bug 1364361 - Part 2: Make AllChildIterator find NAC created by all of an element's anon boxes. https://reviewboard.mozilla.org/r/141524/#review152280 (Sorry for clearing the review flag, I don't really understand MozReview's UI. I'll try to set the flag back to r+.)

I uploaded a broken version of that Part 2 patch, realized, and uploaded a new version about 3 minutes later. Did your analysis run on the old patch? That was the very error I fixed in the new patch.

(In reply to Cameron McCormack (:heycam) from comment #45) > I uploaded a broken version of that Part 2 patch, realized, and uploaded a > new version about 3 minutes later. Did your analysis run on the old patch? > That was the very error I fixed in the new patch. Thanks a lot for this information, your assumption is correct: our bot reported this error only for revision 2 and not for the fixed-up revision 3: - revision 2: https://reviewboard-hg.mozilla.org/gecko/rev/72a24cd9adfe - revision 3: https://reviewboard-hg.mozilla.org/gecko/rev/0812a6454f78 We're considering silencing [clang-diagnostic-error] problems because they're usually unrelated to any patch (it's mostly missing libraries in our bot's environment) but I guess in this case it would have been a legitimate issue to report on a patch.

And passing try this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e45c8653d12e16db34deaaeea4bef63c7b55c045

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. https://reviewboard.mozilla.org/r/138998/#review154098 r=me with the above nits. Sorry for the horrible lag. :( ::: layout/generic/nsFrame.cpp:10500 (Diff revision 6) > + > + AutoTArray<OwnedAnonBox,4> frames; > + AppendDirectlyOwnedAnonBoxes(frames); > + for (OwnedAnonBox& box : frames) { > + if (box.mUpdateStyleFn) { > + box.mUpdateStyleFn(box.mOwningFrame, box.mAnonBoxFrame, Are there any cases here (either branch) in which box.mOwningFrame != this? If not, why do we need box.mOwningFrame at all? In particular, if we don't need an owning frame on box, then we could give OwnedAnonBox a ctor that takes a single nsIFrame* and just append nsIFrame* to the array in most cases... ::: layout/generic/nsIFrame.h:3351 (Diff revision 6) > + , mUpdateStyleFn(aUpdateStyleFn) > + {} > + > + nsIFrame* mOwningFrame; > + nsIFrame* mAnonBoxFrame; > + std::function<UpdateStyleFn> mUpdateStyleFn; Is there a good reason for std::function here instead of just a plain function pointer?

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. https://reviewboard.mozilla.org/r/138998/#review154098 > Are there any cases here (either branch) in which box.mOwningFrame != this? If not, why do we need box.mOwningFrame at all? > > In particular, if we don't need an owning frame on box, then we could give OwnedAnonBox a ctor that takes a single nsIFrame* and just append nsIFrame* to the array in most cases... Yes, any time we "recursively" append directly owned anon boxes under nsIFrame::DoAppendOwnedAnonBoxes we will get box.mOwningFrame != this.

Comment on attachment 8867442 [details] Bug 1364361 - Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. https://reviewboard.mozilla.org/r/138998/#review154098 > Is there a good reason for std::function here instead of just a plain function pointer? No, initially I was using a lambda, but I switched to pointers to static member functions. Will change.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/12db2d364b64 Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. r=bholley,bz https://hg.mozilla.org/integration/autoland/rev/cffde1d0ebb4 Part 2: Make AllChildIterator find NAC created by all of an element's anon boxes. r=bz,janx https://hg.mozilla.org/integration/autoland/rev/0952bb52c59f Part 3: Remove now-unnecessary clearing of ServoElementData from anonymous content destroyer callback. r=bz https://hg.mozilla.org/integration/autoland/rev/9cff9f4e2ef2 Part 4: Add crashtest for a scrollable, multicol <details> element. r=bz

> Yes, any time we "recursively" append directly owned anon boxes under nsIFrame::DoAppendOwnedAnonBoxes OK, but that's not used from DoUpdateStyleOfOwnedAnonBoxes; that calls AppendDirectlyOwnedAnonBoxes. And the consumer of DoAppendOwnedAnonBoxes is AppendNativeAnonymousChildren which doesn't use mOwningFrame at all, right?

Ah, got it. (Because DoUpdateStyleOfOwnedAnonBoxes does the recursion itself, not relying on AppendOwnedAnonBoxes.) I'll remove the mOwningBox in a followup here.

Backed out for heap write hazard: https://hg.mozilla.org/integration/autoland/rev/24d6c162840220434745ded799704aa2c74ec4eb https://hg.mozilla.org/integration/autoland/rev/3b869bea280bb9beac8d6349e465df8811741cea https://hg.mozilla.org/integration/autoland/rev/d82f31d901875472d62ce686be0f1a13575e1f08 https://hg.mozilla.org/integration/autoland/rev/e96f9176cd790a4bebf8f4f7171ac8113c36799d Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9cff9f4e2ef2bfb2fac5538b9668fdb10d702a5b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=107565076&repo=autoland Check the heap write hazards log file. [27.36s] #19 Analyzing Gecko_GetNextStyleChild ... Error: External function Location: _ZN8nsIFrame20AppendOwnedAnonBoxesER8nsTArrayINS_12OwnedAnonBoxEE$void nsIFrame::AppendOwnedAnonBoxes(class nsTArray<nsIFrame::OwnedAnonBox>*) ### SafeArguments: <arg0> Stack Trace: _ZN7mozilla3dom19AllChildrenIterator29AppendNativeAnonymousChildrenEv$void mozilla::dom::AllChildrenIterator::AppendNativeAnonymousChildren() @ dom/base/ChildIterator.cpp#379 ### SafeArguments: this _ZN7mozilla3dom19AllChildrenIterator12GetNextChildEv$nsIContent* mozilla::dom::AllChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp#432 ### SafeArguments: this _ZN7mozilla3dom21StyleChildrenIterator12GetNextChildEv$nsIContent* mozilla::dom::StyleChildrenIterator::GetNextChild() @ dom/base/ChildIterator.cpp#549 ### SafeArguments: <this> Gecko_GetNextStyleChild @ layout/style/ServoBindings.cpp#260 ### SafeArguments: <arg0>

I think it is because AllChildrenIterator::GetNextChild asks nsIFrame::AppendOwnedAnonBoxes to write into the array it's got as a local variable, which is safe. I'll add something to ignoreCallEdge to whitelist this.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e7ec499159f7 Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. r=bholley,bz https://hg.mozilla.org/integration/autoland/rev/e9f7c901e132 Part 2: Make AllChildIterator find NAC created by all of an element's anon boxes. r=bz,janx https://hg.mozilla.org/integration/autoland/rev/65c77754d49c Part 3: Remove now-unnecessary clearing of ServoElementData from anonymous content destroyer callback. r=bz https://hg.mozilla.org/integration/autoland/rev/a05d5d6c04dc Part 4: Add crashtest for a scrollable, multicol <details> element. r=bz

Backed out for bustage at nsIFrame.h:3348: bad implicit conversion constructor for 'OwnedAnonBox': https://hg.mozilla.org/integration/autoland/rev/6e16cb4e08dd0d06016cbc4a30891b046048d9e8 https://hg.mozilla.org/integration/autoland/rev/2f142dc5761f0c53f3ca96cb1ecbdd066a678ccd https://hg.mozilla.org/integration/autoland/rev/4a08aeadf4f3d4ca4bd200cbc0cea1a14174aa3e https://hg.mozilla.org/integration/autoland/rev/028566613d2e4c091b34739d432408f8a619b06a Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a05d5d6c04dc3c92e6ba3f5c66cfebe233ae04e4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=107628277&repo=autoland [task 2017-06-16T09:11:00.408224Z] 09:11:00 INFO - In file included from /home/worker/workspace/build/src/obj-firefox/netwerk/base/Unified_cpp_netwerk_base0.cpp:74: [task 2017-06-16T09:11:00.408314Z] 09:11:00 INFO - In file included from /home/worker/workspace/build/src/netwerk/base/LoadInfo.cpp:12: [task 2017-06-16T09:11:00.409331Z] 09:11:00 INFO - In file included from /home/worker/workspace/build/src/dom/base/nsFrameLoader.h:27: [task 2017-06-16T09:11:00.409413Z] 09:11:00 INFO - /home/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:3348:5: error: bad implicit conversion constructor for 'OwnedAnonBox' [task 2017-06-16T09:11:00.410528Z] 09:11:00 INFO - OwnedAnonBox(nsIFrame* aAnonBoxFrame, [task 2017-06-16T09:11:00.412722Z] 09:11:00 INFO - ^ [task 2017-06-16T09:11:00.412803Z] 09:11:00 INFO - /home/worker/workspace/build/src/obj-firefox/dist/include/nsIFrame.h:3348:5: note: consider adding the explicit keyword to the constructor [task 2017-06-16T09:11:00.412839Z] 09:11:00 INFO - 1 error generated.

Pushed by cmccormack@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d05b78469f18 Part 1: Refactor frame anon box restyling machinery so it can return a list of anon boxes. r=bholley,bz https://hg.mozilla.org/integration/autoland/rev/8d9bab00cc59 Part 2: Make AllChildIterator find NAC created by all of an element's anon boxes. r=bz,janx https://hg.mozilla.org/integration/autoland/rev/3f6c07c56342 Part 3: Remove now-unnecessary clearing of ServoElementData from anonymous content destroyer callback. r=bz https://hg.mozilla.org/integration/autoland/rev/e40f29b5b89c Part 4: Add crashtest for a scrollable, multicol <details> element. r=bz

https://hg.mozilla.org/mozilla-central/rev/d05b78469f18 https://hg.mozilla.org/mozilla-central/rev/8d9bab00cc59 https://hg.mozilla.org/mozilla-central/rev/3f6c07c56342 https://hg.mozilla.org/mozilla-central/rev/e40f29b5b89c