Closed Bug 1158412 Opened 9 years ago Closed 8 years ago

"Assertion failure: false (Unable to find document prescontext and base URI)" with srcset, adoptNode

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox40 --- affected
firefox48 --- fixed

People

(Reporter: jruderman, Assigned: edgar, Mentored)

References

Details

(Keywords: assertion, testcase, Whiteboard: [tw-dom] btpp-active)

Attachments

(3 files)

Attached file testcase
Assertion failure: false (Unable to find document prescontext and base URI), at dom/base/ResponsiveImageSelector.cpp:296
Attached file stack
Nothing ever requires a document to have a prescontext, so asserting that it does is really not ok...
Flags: needinfo?(josh)
(In reply to Not doing reviews right now from comment #2)
> Nothing ever requires a document to have a prescontext, so asserting that it
> does is really not ok...

This is my fault - The pres context is used below getting the current CSS-to-dev-pixels ratio:

> double displayDensity = pctx->CSSPixelsToDevPixels(1.0f);

Ideally for responsive content we'd use a best guess (parent/root window?) before just falling back to 1.0. Finding the best image (and what density we choose to compare against) is still left up to the UA in the spec

There's a similar use in ComputeFinalWidthForCurrentViewport for app units -> css pixels
Assignee: nobody → josh
Flags: needinfo?(josh)
Assignee: josh → nobody
Mentor: josh
Whiteboard: [tw-dom]
That particular assertion seems to have been removed, but there's another similar one in ComputeFinalWidthForCurrentViewport.
(In reply to Josh Matthews [:jdm] from comment #5)
> That particular assertion seems to have been removed, but there's another
> similar one in ComputeFinalWidthForCurrentViewport.

We won't have chance to hit the assertion in ComputeFinalWidthForCurrentViewport(), because the SelectImage returns earlier before calling into ComputeFinalWidthForCurrentViewport();
Attached patch Patch, v1Splinter Review
Assignee: nobody → echen
Whiteboard: [tw-dom] → [tw-dom] btpp-active
Comment on attachment 8741840 [details] [diff] [review]
Patch, v1

Review of attachment 8741840 [details] [diff] [review]:
-----------------------------------------------------------------

As I mentioned in comment #6, it is safe to remove the assertion in ComputeFinalWidthForCurrentViewport() and I still add a crash test for them.
Attachment #8741840 - Flags: review?(josh)
Attachment #8741840 - Flags: review?(josh) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/038ff8933b43
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(In reply to Edgar Chen [:edgar][:echen] from comment #6)
> (In reply to Josh Matthews [:jdm] from comment #5)
> > That particular assertion seems to have been removed, but there's another
> > similar one in ComputeFinalWidthForCurrentViewport.
> 
> We won't have chance to hit the assertion in
> ComputeFinalWidthForCurrentViewport(), because the SelectImage returns
> earlier before calling into ComputeFinalWidthForCurrentViewport();

per comment 2, is SelectImage() bailing here proper? Seems like there's an edge case bug here where SelectImage() runs without a pres context and we skip loading any image. Ideally we'd select a best-effort-without-dpi image (lowest density? closest to 1.0?) and ensure we fire a DPI-changed callback when that information is available.
Flags: needinfo?(echen)
(In reply to John Schoenick [:johns] from comment #12)
> per comment 2, is SelectImage() bailing here proper? Seems like there's an
> edge case bug here where SelectImage() runs without a pres context and we
> skip loading any image. Ideally we'd select a best-effort-without-dpi image
> (lowest density? closest to 1.0?) and ensure we fire a DPI-changed callback
> when that information is available.

File bug 1266318 for this. Thank you, John.
Flags: needinfo?(echen)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.